Thread: SQL/JSON: functions

Attached patches implementing all SQL/JSON functions excluding JSON_TABLE:

JSON_OBJECT()
JSON_OBJECTAGG()
JSON_ARRAY()
JSON_ARRAYAGG()

JSON_EXISTS()
JSON_VALUE()
JSON_QUERY()

IS JSON predicate


This patchset depends on 8th version of jsonpath patchset that was 
posted in 
https://www.postgresql.org/message-id/48f13b75-0be7-c356-ff26-1db743add56e%40postgrespro.ru 


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

Attached 9th version of SQL/JSON patches rebased onto the latest master.

Displayed column name for SQL/JSON functions was fixed.

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 SQL/JSON patches rebased onto the latest master.

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

Attached 11th version of SQL/JSON patches rebased onto the latest master.

Fixed uninitialized FORMAT JSON location in gram.y and column names for 
JSON constructors.

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

Attached 12th version of SQL/JSON patches rebased onto the latest master.

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

On 7 March 2018 at 14:34, Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:

> Attached 12th version of SQL/JSON patches rebased onto the latest master.

Please write some docs or notes to go with this.

If you drop a big pile of code with no explanation it will just be ignored.

I think many people want SQL/JSON, but the purpose of a patch
submission is to allow a committer to review and commit without
needing to edit anything. It shouldn't be like assembling flat pack
furniture while wearing a blindfold.

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Mar 13, 2018 at 2:04 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 7 March 2018 at 14:34, Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
>
>> Attached 12th version of SQL/JSON patches rebased onto the latest master.
>
> Please write some docs or notes to go with this.
>
> If you drop a big pile of code with no explanation it will just be ignored.
>
> I think many people want SQL/JSON, but the purpose of a patch
> submission is to allow a committer to review and commit without
> needing to edit anything. It shouldn't be like assembling flat pack
> furniture while wearing a blindfold.

The docs are here
https://github.com/obartunov/sqljsondoc/blob/master/README.jsonpath.md

It's not easy to write docs for SQL/JSON in xml, so I decided to write in more
friendly way. We'll have time to convert it to postgres format.


>
> --
> Simon Riggs                http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Mar 13, 2018 at 04:08:01PM +0300, Oleg Bartunov wrote:
> The docs are here
> https://github.com/obartunov/sqljsondoc/blob/master/README.jsonpath.md
>
> It's not easy to write docs for SQL/JSON in xml, so I decided to write in more
> friendly way. We'll have time to convert it to postgres format.

If you aim at getting a feature committed first without its
documentation, and getting the docs written after the feature freeze
using a dedicated open item or such, this is much acceptable in my
opinion and the CF is running short in time.
--
Michael

On 2018-03-14 07:54:35 +0900, Michael Paquier wrote:
> On Tue, Mar 13, 2018 at 04:08:01PM +0300, Oleg Bartunov wrote:
> > The docs are here
> > https://github.com/obartunov/sqljsondoc/blob/master/README.jsonpath.md
> > 
> > It's not easy to write docs for SQL/JSON in xml, so I decided to write in more
> > friendly way. We'll have time to convert it to postgres format.
> 
> If you aim at getting a feature committed first without its
> documentation, and getting the docs written after the feature freeze
> using a dedicated open item or such, this is much acceptable in my
> opinion and the CF is running short in time.

Given that this patch still uses PG_TRY/CATCH around as wide paths of
code as a whole ExecEvalExpr() invocation, basically has gotten no
review, I don't see this going anywhere for v11.

Greetings,

Andres Freund

Attached 13th version of the patches:

* Subtransactions in PG_TRY/CATCH in ExecEvalJsonExpr() were made unconditional, regardless of the volatility of expressions.

* PG_TRY/CATCH in ExecEvalExprPassingCaseValue() was removed along with the entire function.

On 15.03.2018 11:08, Oleg Bartunov wrote:

On 14 Mar 2018 17:11, "Alexander Korotkov" <a.korotkov@postgrespro.ru> wrote:

On Wed, Mar 14, 2018 at 2:10 AM, Andres Freund <andres@anarazel.de> wrote:

On 2018-03-14 07:54:35 +0900, Michael Paquier wrote:
> On Tue, Mar 13, 2018 at 04:08:01PM +0300, Oleg Bartunov wrote:
> > The docs are here
> > https://github.com/obartunov/sqljsondoc/blob/master/README.jsonpath.md
> >
> > It's not easy to write docs for SQL/JSON in xml, so I decided to write in more
> > friendly way. We'll have time to convert it to postgres format.
>
> If you aim at getting a feature committed first without its
> documentation, and getting the docs written after the feature freeze
> using a dedicated open item or such, this is much acceptable in my
> opinion and the CF is running short in time.

Given that this patch still uses PG_TRY/CATCH around as wide paths of
code as a whole ExecEvalExpr() invocation,

 

I agree that we should either use PG_TRY/CATCH over some small and safe

codepaths or surround PG_TRY/CATCH with subtransactions.  PG_TRY/CATCH over

ExecEvalExpr() looks really unacceptable.

basically has gotten no
review, I don't see this going anywhere for v11.

I wouldn't be co categorical at this point.  Patchset is there for about year.

Some parts of code received more of review while some parts receives less.

We can surround all dangerous PG_TRY/CATCH pairs with subtransactions,

tolerate performance penalty and leave further optimizations for future releases.

Agree it's not difficult.

In worst case, we can remove codepaths which use PG_TRY/CATCH and

leave only ERROR ON ERROR behavior of SQL/JSON.

No-no, json user will be really upset on this. Our goal is to be the first relational database with strong standard compliance.

On 15.03.2018 20:04, Nikita Glukhov wrote:
> Attached 13th version of the patches:
>
> * Subtransactions in PG_TRY/CATCH in ExecEvalJsonExpr() were made unconditional,
>   regardless of the volatility of expressions.
>
> * PG_TRY/CATCH in ExecEvalExprPassingCaseValue() was removed along with the
>   entire function.


Attached 15th version of the patches:
  * disabled parallel execution of SQL/JSON query functions when internal
    subtransactions are used (if ERROR ON ERROR is not specified)
  * added experimental optimization of internal subtransactions (see below)


The new patch #14 is an experimental attempt to reduce overhead of
subtransaction start/commit which can result in 2x-slowdown in the simplest
cases.  By the idea of Alexander Korotkov, subtransaction is not really
committed if it has not touched the database and its XID has not been assigned
(DB modification is not expected in type casts functions) and then can be reused
when the next subtransaction is started.  So, all rows in JsonExpr can be
executed in the single cached subtransaction.  This optimization really helps
to reduce overhead from 100% to 5-10%:

-- without subtransactions
=# EXPLAIN ANALYZE
    SELECT JSON_VALUE('true'::jsonb, '$' RETURNING boolean ERROR ON ERROR)
    FROM generate_series(1, 10000000) i;
  ...
  Execution Time: 2785.410 ms

-- cached subtransactions
=# EXPLAIN ANALYZE
    SELECT JSON_VALUE('true'::jsonb, '$' RETURNING boolean)
    FROM generate_series(1, 10000000) i;
  ...
  Execution Time: 2939.363 ms

-- ordinary subtransactions
=# EXPLAIN ANALYZE
    SELECT JSON_VALUE('true'::jsonb, '$' RETURNING boolean)
    FROM generate_series(1, 10000000) i;
  ...
  Execution Time: 5417.268 ms


But, unfortunately, I don't believe that this patch is completely correct,
mainly because the behavior of subtransaction callbacks (and their expectations
about subtransaction's lifecycle too) seems unpredictable to me.



Even with this optimization, internal subtransactions still have one major
drawback -- they disallow parallel query execution, because background
workers do not support subtransactions now.  Example:

=# CREATE TABLE test_parallel_json_value AS
    SELECT i::text::jsonb AS js FROM generate_series(1, 5000000) i;
CREATE TABLE

=# EXPLAIN ANALYZE
    SELECT sum(JSON_VALUE(js, '$' RETURNING numeric ERROR ON ERROR))
    FROM test_parallel_json_value;
                                                                QUERY PLAN

-----------------------------------------------------------------------------------------------------------------------------------------
  Finalize Aggregate  (cost=79723.15..79723.16 rows=1 width=32) (actual time=455.062..455.062 rows=1 loops=1)
    ->  Gather  (cost=79722.93..79723.14 rows=2 width=32) (actual time=455.052..455.055 rows=3 loops=1)
          Workers Planned: 2
          Workers Launched: 2
          ->  Partial Aggregate  (cost=78722.93..78722.94 rows=1 width=32) (actual time=446.000..446.000 rows=1
loops=3)
                ->  Parallel Seq Scan on t  (cost=0.00..52681.30 rows=2083330 width=18) (actual time=0.023..104.779
rows=1666667loops=3)
 
  Planning Time: 0.044 ms
  Execution Time: 456.460 ms
(8 rows)

=# EXPLAIN ANALYZE
    SELECT sum(JSON_VALUE(js, '$' RETURNING numeric))
    FROM test_parallel_json_value;
                                                      QUERY PLAN
--------------------------------------------------------------------------------------------------------------------
  Aggregate  (cost=144347.82..144347.83 rows=1 width=32) (actual time=1381.938..1381.938 rows=1 loops=1)
    ->  Seq Scan on t  (cost=0.00..81847.92 rows=4999992 width=18) (actual time=0.076..309.676 rows=5000000 loops=1)
  Planning Time: 0.082 ms
  Execution Time: 1384.133 ms
(4 rows)


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

Attached 16th version of the patches:* changed type of new SQL keyword STRING   (STRING is used as a function parameter name in Pl/Tcl tests)* removed implicit coercion via I/O from JSON_VALUE (see below)

2018-06-28 2:18 GMT+02:00 Nikita Glukhov <n.gluhov@postgrespro.ru>:

On 15.03.2018 20:04, Nikita Glukhov wrote:

Attached 13th version of the patches:

* Subtransactions in PG_TRY/CATCH in ExecEvalJsonExpr() were made unconditional,
  regardless of the volatility of expressions.

* PG_TRY/CATCH in ExecEvalExprPassingCaseValue() was removed along with the
  entire function.

Attached 15th version of the patches:
 * disabled parallel execution of SQL/JSON query functions when internal
   subtransactions are used (if ERROR ON ERROR is not specified)
 * added experimental optimization of internal subtransactions (see below)


The new patch #14 is an experimental attempt to reduce overhead of
subtransaction start/commit which can result in 2x-slowdown in the simplest
cases.  By the idea of Alexander Korotkov, subtransaction is not really
committed if it has not touched the database and its XID has not been assigned
(DB modification is not expected in type casts functions) and then can be reused
when the next subtransaction is started.  So, all rows in JsonExpr can be
executed in the single cached subtransaction.  This optimization really helps
to reduce overhead from 100% to 5-10%:

I read a technical report for SQL/JSON. If I understand it well, then ON ERROR clause is primary related to structural errors, not to all errors.

So your implementation is maybe too tolerant, what has this issue. There was not any example, so this clause should to handle cast errors or any other errors than JSON structural.

The playing with other implementation of subtransactions doesn't look like safe way, more if it is not necessary

The other possible error are casts errors. We can introduce new exception safe input functions. These functions can be interesting for fault tolerant COPY for example.

SQL/JSON standard requires handling of cast errors too.

9.40 Casting an SQL/JSON sequence to an SQL type (pages 724-725):

4) If TEMPST is successful completion, then: b) If the length of SEQ is 1 (one), then let I be the SQL/JSON item in SEQ.   Case:     ...   iii) Otherwise, let IDT be the data type of I.     Case:     1) If IDT cannot be cast to target type DT according to the Syntax Rules        of Subclause 6.13, "<cast specification>", then let TEMPST be data        exception — SQL/JSON item cannot be cast to target type.     2) Otherwise, let X be an SQL variable whose value is I. Let V be the        value of CAST (X AS DT).  If an exception condition is raised by this        <cast specification>, then let TEMPST be that exception condition. ...
5) Case: a) If TEMPST is successful completion, then let OUTST be successful    completion. b) If ONERROR is ERROR, then let OUTST be TEMPST. c) If ONERROR is NULL, then let V be the SQL null value and let OUTST be    successful completion. d) If ONERROR immediately contains DEFAULT, then let VE be the    <value expression> immediately contained in ONERROR. Let V be the value of    CAST (VE AS DT)   Case:   i) If an exception condition is raised by this <cast specification>, then      let OUTST be that exception condition.   ii) Otherwise, let OUTST be successful completion.


In 4.b.iii.1 said that there should be an error if the desired cast does not exist.
In the previous versions of the patches there was implicit coercion via I/O here 
instead of error, so I decided to fix it the last version (fix is combined with a
minor refactoring of ExecEvalJsonExpr()).

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

> On Tue, Jul 3, 2018 at 2:57 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> 2018-07-03 14:30 GMT+02:00 Nikita Glukhov <n.gluhov@postgrespro.ru>:
>>
>> Attached 16th version of the patches:
>>  * changed type of new SQL keyword STRING
>>    (STRING is used as a function parameter name in Pl/Tcl tests)
>>  * removed implicit coercion via I/O from JSON_VALUE (see below)

Unfortunately, the current version of patch 0010-add-invisible-coercion-form
doesn't not apply anymore without conflicts, could you please rebase it?

On 26.11.2018 15:57, Dmitry Dolgov wrote:

>> On Tue, Jul 3, 2018 at 2:57 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>>
>> 2018-07-03 14:30 GMT+02:00 Nikita Glukhov <n.gluhov@postgrespro.ru>:
>>> Attached 16th version of the patches:
>>>   * changed type of new SQL keyword STRING
>>>     (STRING is used as a function parameter name in Pl/Tcl tests)
>>>   * removed implicit coercion via I/O from JSON_VALUE (see below)
> Unfortunately, the current version of patch 0010-add-invisible-coercion-form
> doesn't not apply anymore without conflicts, could you please rebase it?

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


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

> On Mon, Nov 26, 2018 at 5:11 PM Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
>
> On 26.11.2018 15:57, Dmitry Dolgov wrote:
>
> >> On Tue, Jul 3, 2018 at 2:57 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> >>
> >> 2018-07-03 14:30 GMT+02:00 Nikita Glukhov <n.gluhov@postgrespro.ru>:
> >>> Attached 16th version of the patches:
> >>>   * changed type of new SQL keyword STRING
> >>>     (STRING is used as a function parameter name in Pl/Tcl tests)
> >>>   * removed implicit coercion via I/O from JSON_VALUE (see below)
> > Unfortunately, the current version of patch 0010-add-invisible-coercion-form
> > doesn't not apply anymore without conflicts, could you please rebase it?
>
> Attached 20th version of the patches rebased onto the current master.

Thank you,

Unfortunately, looks like we're in the loop "Rebase. Wait. Repeat", since the
patch again has some conflicts (this time I hope just minor conflicts). Which
is probably understandable taking into account the size of it. Maybe we need to
think about how to proceed with this, do you have any ideas?

On Mon, Nov 26, 2018 at 5:11 PM Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:

On 26.11.2018 15:57, Dmitry Dolgov wrote:

On Tue, Jul 3, 2018 at 2:57 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:

2018-07-03 14:30 GMT+02:00 Nikita Glukhov <n.gluhov@postgrespro.ru>:

Attached 16th version of the patches: * changed type of new SQL keyword STRING   (STRING is used as a function parameter name in Pl/Tcl tests) * removed implicit coercion via I/O from JSON_VALUE (see below)

Unfortunately, the current version of patch 0010-add-invisible-coercion-form
doesn't not apply anymore without conflicts, could you please rebase it?

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

Thank you,

Unfortunately, looks like we're in the loop "Rebase. Wait. Repeat", since the
patch again has some conflicts (this time I hope just minor conflicts). Which
is probably understandable taking into account the size of it. Maybe we need to
think about how to proceed with this, do you have any ideas?

Attached 21st version of the patches.

I decided to include here patch 0000 with complete jsonpath implementation (it
is a squash of all 6 jsonpath-v21 patches). I hope this will simplify reviewing
and testing in cfbot.cputube.org.


If it will conflict again, then you can 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)

 > Attached 21st version of the patches.
 >
 > I decided to include here patch 0000 with complete jsonpath 
implementation (it
 > is a squash of all 6 jsonpath-v21 patches). I hope this will simplify 
reviewing
 > and testing in cfbot.cputube.org.

I'd like to help in reviewing this patch. Please let me know if there's 
something in particular I should focus on such as documentation, 
functionality, or source. If not, I'll probably just proceed in that order.

Regards, Andy Alsup

 > Attached patches implementing all SQL/JSON functions excluding 
JSON_TABLE:
 >
 > JSON_OBJECT()
 > JSON_OBJECTAGG()
 > JSON_ARRAY()
 > JSON_ARRAYAGG()
 >
 > JSON_EXISTS()
 > JSON_VALUE()
 > JSON_QUERY()

Sorry if this is a stupid question, but is this patch intended to 
implement any SQL/JSON functions? I'm basing this question on the 
original patch post (quoted above). The patch appears to be focused 
exclusively on "jsonpath expressions". I only ask because I think I 
misinterpreted and spent some time wondering if I had missed a patch file.

So far, the jsonpath docs are very readable; however, I think a few 
complete examples (full SELECT statements) of using jsonpath expressions 
would be helpful to someone new to this technology. Does postgresql 
provide a sample schema, similar to the Or*cle scott/tiger (emp/dept) 
schema, we could use for examples? Alternatively, we could reference 
something mentioned at 
https://wiki.postgresql.org/wiki/Sample_Databases. I think it would be 
nice if all the docs referenced the same schema, when possible.

For tests, would it be helpful to have some tests that 
demonstrate/assert equality between "jsonb operators" and "jsonpath 
expressions"? For example, using the existing regression test data the 
following should assert equality in operator vs. expression:

SELECT
   CASE WHEN jop_count = expr_count THEN 'pass' ELSE 'fail' END
FROM
   (
     -- jsonb operator
     SELECT count(*)
     FROM testjsonb
     WHERE j->>'abstract' LIKE 'A%'
   ) as jop_count,
   (
     -- jsonpath expression
     SELECT count(*)
     FROM testjsonb
     WHERE j @? '$.abstract ? (@ starts with "A")'
   ) as expr_count;

Hi,

On 2018-12-05 02:01:19 +0300, Nikita Glukhov wrote:

> +    if (!PG_ARGISNULL(1) &&
> +        strncmp("any", VARDATA(type), VARSIZE_ANY_EXHDR(type)))
> +    {
> +        JsonLexContext *lex;
> +        JsonTokenType tok;
> +
> +        lex = makeJsonLexContext(json, false);
> +
> +        /* Lex exactly one token from the input and check its type. */
> +        PG_TRY();
> +        {
> +            json_lex(lex);
> +        }
> +        PG_CATCH();
> +        {
> +            if (ERRCODE_TO_CATEGORY(geterrcode()) == ERRCODE_DATA_EXCEPTION)
> +            {
> +                FlushErrorState();
> +                MemoryContextSwitchTo(mcxt);
> +                PG_RETURN_BOOL(false);    /* invalid json */
> +            }
> +            PG_RE_THROW();
> +        }
> +        PG_END_TRY();


It baffles me that a year after I raised this as a serious issue, in
this thread, this patch still contains code like this.


Greetings,

Andres Freund

Attached 34th version of the patches.

On 16.02.2019 8:12, Andres Freund wrote:

On 2018-12-05 02:01:19 +0300, Nikita Glukhov wrote:

+		JsonLexContext *lex;
+		JsonTokenType tok;
+
+		lex = makeJsonLexContext(json, false);
+
+		/* Lex exactly one token from the input and check its type. */
+		PG_TRY();
+		{
+			json_lex(lex);
+		}
+		PG_CATCH();
+		{
+			if (ERRCODE_TO_CATEGORY(geterrcode()) == ERRCODE_DATA_EXCEPTION)
+			{
+				FlushErrorState();
+				MemoryContextSwitchTo(mcxt);
+				PG_RETURN_BOOL(false);	/* invalid json */
+			}
+			PG_RE_THROW();
+		}
+		PG_END_TRY();

It baffles me that a year after I raised this as a serious issue, in
this thread, this patch still contains code like this.

PG_TRY/PG_CATCH was removed here: 'throwErrors' flag was added to JsonLexContext 
instead.

Also usage of subtransactions is SQL/JSON functions (JsonExpr node) was
optimized: they can be not only omitted in ERROR ON ERROR case but also when
the resulting conversion from the SQL/JSON item type to the target SQL type is
no-op.


Below are the results of simple performance test
(operator #>> uses optimization which I recently posted in the separate patch):
                       query                           | time, ms
-------------------------------------------------------------------JSON_VALUE(js, '$.x.y.z' RETURNING text) = '123'       | 1923,360 (subtrans!)JSON_VALUE(js, '$.x.y.z' RETURNING text                         ERROR ON ERROR) = '123'       |  970,604JSON_VALUE(js, '$.x.y.z' RETURNING numeric) = '123'    |  792,412 JSON_VALUE(js, '$.x.y.z' RETURNING numeric                         ERROR ON ERROR) = '123'       |  786,647
(js->'x'->'y'->'z') = '123'                            | 1104,470(js->'x'->'y'->'z')::numeric = '123'                   |  940,037(js->'x'->'y'->>'z') = '123'                           |  688,484
(js #>  '{x,y,z}') = '123'                             | 1127,661(js #>  '{x,y,z}')::numeric = '123'                    |  971,931(js #>> '{x,y,z}') = '123'                             |  718,173


Table with jsonb rows like  '{"x": {"y": {"z": 123}}}':
CREATE TABLE t AS
SELECT JSON_OBJECT('x' : JSON_OBJECT('y' : JSON_OBJECT('z' : i))) js
FROM generate_series(1, 3000000) i;

Example query:
SELECT * FROM t WHERE JSON_VALUE(js, '$.x.y.z' RETURNING numeric) = '123';

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

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

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

On 3/5/19 5:35 PM, Nikita Glukhov wrote:
> Attached 36th version of the patches rebased onto jsonpath v36.
While testing this patch a found a few issues:

[1] I was not able to apply the patch to the current HEAD. However, it 
applies cleanly to commit: e988878f85 (NOTE: I did not investigate which 
commit between e988878f85 and HEAD caused problems).

[2] JsonPath array slicing does not work. I'm not aware of a 
comprehensive list of JsonPath features/syntax that is targeted for 
support; however, I did try various forms of array slicing, which don't 
currently work.

Here are a few examples:

The input document is the same in each example.

{
   "a1": 123,
   "b1": "xxx",
   "c1": {
     "a2": 456,
     "b2": "yyy",
     "c2": [
       {"a3": 777, "b3": "7z"},
       {"a3": 888, "b3": "8z"}
     ]
   }
}

array wildcard selector [*] works: $.c1.c2[*].b3

# select json_path_query('{"a1": 123, "b1": "xxx", "c1": {"a2": 456, 
"b2": "yyy", "c2": [{"a3": 777, "b3": "7z"},{"a3": 888, "b3": 
"8z"}]}}'::json, '$.c1.c2[*].b3'::jsonpath);
  json_path_query
-----------------
  "7z"
  "8z"
(2 rows)

array index selector [0] works: $.c1.c2[0].b3

jsonpatch=# select json_path_query('{"a1": 123, "b1": "xxx", "c1": 
{"a2": 456, "b2": "yyy", "c2": [{"a3": 777, "b3": "7z"},{"a3": 888, 
"b3": "8z"}]}}'::json, '$.c1.c2[0].b3'::jsonpath);
  json_path_query
-----------------
  "7z"
(1 row)

array slicing [0:], [:1], and [0:1] do not work:$.c1.c2[0:].b3

# select json_path_query('{"a1": 123, "b1": "xxx", "c1": {"a2": 456, 
"b2": "yyy", "c2": [{"a3": 777, "b3": "7z"},{"a3": 888, "b3": 
"8z"}]}}'::json, '$.c1.c2[0:].b3'::jsonpath);
2019-05-13 20:47:48.740 EDT [21856] ERROR:  bad jsonpath representation 
at character 147
2019-05-13 20:47:48.740 EDT [21856] DETAIL:  syntax error, unexpected 
':', expecting ',' or ']' at or near ":"
2019-05-13 20:47:48.740 EDT [21856] STATEMENT:  select 
json_path_query('{"a1": 123, "b1": "xxx", "c1": {"a2": 456, "b2": "yyy", 
"c2": [{"a3": 777, "b3": "7z"},{"a3": 888, "b3": "8z"}]}}'::json, 
'$.c1.c2[0:].b3'::jsonpath);
ERROR:  bad jsonpath representation
LINE 1: ...7, "b3": "7z"},{"a3": 888, "b3": "8z"}]}}'::json, '$.c1.c2[0...
                                                              ^
DETAIL:  syntax error, unexpected ':', expecting ',' or ']' at or near ":"

Hi!

On Tue, May 14, 2019 at 3:54 AM Andrew Alsup <bluesbreaker@gmail.com> wrote:
> array slicing [0:], [:1], and [0:1] do not work:$.c1.c2[0:].b3
>
> # select json_path_query('{"a1": 123, "b1": "xxx", "c1": {"a2": 456,
> "b2": "yyy", "c2": [{"a3": 777, "b3": "7z"},{"a3": 888, "b3":
> "8z"}]}}'::json, '$.c1.c2[0:].b3'::jsonpath);
> 2019-05-13 20:47:48.740 EDT [21856] ERROR:  bad jsonpath representation
> at character 147
> 2019-05-13 20:47:48.740 EDT [21856] DETAIL:  syntax error, unexpected
> ':', expecting ',' or ']' at or near ":"
> 2019-05-13 20:47:48.740 EDT [21856] STATEMENT:  select
> json_path_query('{"a1": 123, "b1": "xxx", "c1": {"a2": 456, "b2": "yyy",
> "c2": [{"a3": 777, "b3": "7z"},{"a3": 888, "b3": "8z"}]}}'::json,
> '$.c1.c2[0:].b3'::jsonpath);
> ERROR:  bad jsonpath representation
> LINE 1: ...7, "b3": "7z"},{"a3": 888, "b3": "8z"}]}}'::json, '$.c1.c2[0...
>                                                               ^
> DETAIL:  syntax error, unexpected ':', expecting ',' or ']' at or near ":"

There is no color syntax from array slicing in jsonpath.  Instead of
"[0:]" one should write "[0 to last]".  See documentation
https://www.postgresql.org/docs/devel/datatype-json.html#TYPE-JSONPATH-ACCESSORS

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

On Tue, May 14, 2019 at 12:54 PM Andrew Alsup <bluesbreaker@gmail.com> wrote:
> On 3/5/19 5:35 PM, Nikita Glukhov wrote:
> > Attached 36th version of the patches rebased onto jsonpath v36.
> While testing this patch a found a few issues:
>
> [1] I was not able to apply the patch to the current HEAD. However, it
> applies cleanly to commit: e988878f85 (NOTE: I did not investigate which
> commit between e988878f85 and HEAD caused problems).

Thanks for that note, which should help other reviewers/testers
looking a this patch set in CF1.  I hope we can eventually get a
rebased patch set, though.

-- 
Thomas Munro
https://enterprisedb.com

On 15.07.2019 5:35, Thomas Munro wrote:

> On Tue, May 14, 2019 at 12:54 PM Andrew Alsup <bluesbreaker@gmail.com> wrote:
>> On 3/5/19 5:35 PM, Nikita Glukhov wrote:
>>> Attached 36th version of the patches rebased onto jsonpath v36.
>> While testing this patch a found a few issues:
>>
>> [1] I was not able to apply the patch to the current HEAD. However, it
>> applies cleanly to commit: e988878f85 (NOTE: I did not investigate which
>> commit between e988878f85 and HEAD caused problems).
> Thanks for that note, which should help other reviewers/testers
> looking a this patch set in CF1.  I hope we can eventually get a
> rebased patch set, though.
>
Attached 37th version of the patches rebased onto current master.

Preliminary patch #1 contains:
  - implementation of jsonpath .datetime() method (see [1])
  - jsonpath support for json type (should be posted later in a separate 
thread)


[1] 
https://www.postgresql.org/message-id/CAPpHfduLcTtOx5dqs6ehDVy2cbLDci9JTkKdwFf1DzSy5dMjBA%40mail.gmail.com

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

On 2019-Jul-16, Nikita Glukhov wrote:

> Attached 37th version of the patches rebased onto current master.
> 
> Preliminary patch #1 contains:
>  - implementation of jsonpath .datetime() method (see [1])
>  - jsonpath support for json type (should be posted later in a separate
> thread)

0001 doesn't apply anymore.  Can you please rebase?

(Only now I realize that this SQL/JSON stuff has its own commitfest
entry, separate from the JSON_TABLE one.)

Thanks,

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

On 04.09.2019 1:27, Alvaro Herrera wrote:

> On 2019-Jul-16, Nikita Glukhov wrote:
>
>> Attached 37th version of the patches rebased onto current master.
>>
>> Preliminary patch #1 contains:
>>   - implementation of jsonpath .datetime() method (see [1])
>>   - jsonpath support for json type (should be posted later in a separate
>> thread)
> 0001 doesn't apply anymore.  Can you please rebase?
>
> (Only now I realize that this SQL/JSON stuff has its own commitfest
> entry, separate from the JSON_TABLE one.)
>
> Thanks,

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

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

On 17.09.2019 0:28, Nikita Glukhov wrote:

> On 04.09.2019 1:27, Alvaro Herrera wrote:
>
>> On 2019-Jul-16, Nikita Glukhov wrote:
>>
>>> Attached 37th version of the patches rebased onto current master.
>>>
>>> Preliminary patch #1 contains:
>>>   - implementation of jsonpath .datetime() method (see [1])
>>>   - jsonpath support for json type (should be posted later in a 
>>> separate
>>> thread)
>> 0001 doesn't apply anymore.  Can you please rebase?
>>
>> (Only now I realize that this SQL/JSON stuff has its own commitfest
>> entry, separate from the JSON_TABLE one.)
>>
>> Thanks,
>
> Attached 38th version of the patches rebased onto current master.
>
Sorry, attached new edition of 38th version with minor corrections
described in "SQL/JSON: JSON_TABLE" thread.


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

This has recently been broken -- please rebase.  (JSON_TABLE likewise).

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

On 25.09.2019 23:55, Alvaro Herrera wrote:

> This has recently been broken -- please rebase.  (JSON_TABLE likewise).

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

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

On 9/27/19 9:42 PM, Nikita Glukhov wrote:
> Attached 39th version of the patches rebased onto current master.
>
> -- 
> Nikita Glukhov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
I am unable to cleanly apply this patch.
I've tried applying to the current master (4f4061b2dd)
I've also tried apply to commit b81a9c2fc5 back through 709d003fbd, with 
no success.

Can you please tell me to which commit I can apply the patch? Is the 
following command correct for applying the patch files?

`patch -p1 < ~/Downloads/0001-Jsonpath-support-for-json-v39.patch`

Thanks for your help,
Andrew Alsup

On 21.10.2019 19:00, Andrew Alsup wrote:
> On 9/27/19 9:42 PM, Nikita Glukhov wrote:
>> Attached 39th version of the patches rebased onto current master.
> I am unable to cleanly apply this patch.
> I've tried applying to the current master (4f4061b2dd)
> I've also tried apply to commit b81a9c2fc5 back through 709d003fbd, 
> with no success.
>
> Can you please tell me to which commit I can apply the patch? Is the 
> following command correct for applying the patch files?
>
> `patch -p1 < ~/Downloads/0001-Jsonpath-support-for-json-v39.patch`
>
> Thanks for your help,
> Andrew Alsup
>
v39 patch is based on 5ee96b3e2221d154ffcb719bd2dee1179c53f821

Use the following git command to apply patches:

git am ~/Downloads/0001-Jsonpath-support-for-json-v39.patch

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

On 10/21/19 12:44 PM, Nikita Glukhov wrote:
>
> v39 patch is based on 5ee96b3e2221d154ffcb719bd2dee1179c53f821
>
> Use the following git command to apply patches:
>
> git am ~/Downloads/0001-Jsonpath-support-for-json-v39.patch
>

Thank you. The patch applied fine, with no errors.

Is this the type of testing that would be helpful? I plan to construct a 
number of separate queries to test more nuanced edge cases. This test 
simply compares the output of 4 separate sub-queries that should provide 
the same results.

SELECT
   CASE WHEN jop = jp_expr AND jop = jval AND jop = jval_path
     THEN 'pass'
     ELSE 'fail'
   END
FROM
   (
     -- jsonb operator
     SELECT count(*)
     FROM testjsonb
     WHERE j->>'abstract' LIKE 'A%'
   ) as jop,
   (
     -- jsonpath expression
     SELECT count(*)
     FROM testjsonb
     WHERE j @? '$.abstract ? (@ starts with "A")'
   ) as jp_expr,
   (
     -- json_value()
     SELECT count(*)
     FROM testjsonb
     WHERE JSON_VAlUE(j, 'lax $.abstract') LIKE 'A%'
   ) as jval,
   (
     -- json_value(jsonpath)
     SELECT count(*)
     FROM testjsonb
     WHERE JSON_VALUE(j, 'lax $.abstract ? (@ starts with "A")') IS NOT NULL
   ) as jval_path;


If I'm completely off base for how testing is normally conducted, please 
let me know.

Thanks,
Andrew Alsup

Attached 40th version of the patches.


I have added some documentation which has been simply copied from [1].

Also, I have split patches more correctly, and extracted patch #2 with common
SQL/JSON clauses, that are used by both constructors and query functions.
Patches #3 and #4 are necessary only for constructors (patch #5).
Patches #5 and #6 are almost independent on patch #1, which is needed only for
query functions.

So, patches #5, #6, #7 are independent now, but the code changes in them are 
still conflicting.  I can rebase #6 and #7 on top of #2, if it is necessary 
for separate review/commit.


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

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

Attached 41th version of the patches.

Changes since previous version:* Enabled DEFAULT clause for ON ERROR/ON EMPTY behaviors in JSON_QUERY()* Added RETURNING clause to JSON_EXISTS() ("side effect" of implementation   EXISTS PATH columns in JSON_TABLE)* ARRAY in EMPTY ARRAY ON ERROR clause is optional now for better Oracle   compatibility

On 17.01.2020 9:54, Pavel Stehule wrote:

On 14. 11. 2019 v 17:46 Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:

Attached 40th version of the patches.

I have added some documentation which has been simply copied from [1].

Also, I have split patches more correctly, and extracted patch #2 with common
SQL/JSON clauses, that are used by both constructors and query functions.
Patches #3 and #4 are necessary only for constructors (patch #5).
Patches #5 and #6 are almost independent on patch #1, which is needed only for
query functions.

So, patches #5, #6, #7 are independent now, but the code changes in them are 
still conflicting.  I can rebase #6 and #7 on top of #2, if it is necessary 
for separate review/commit.


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

I tested cumulative patch - sent in json_table patch.

I almost satisfied by quality of this patch. There is very good conformance with standard and with Oracle. Unfortunately MySQL in this part of JSON support is not compatible.

I found one issue, when I tested some examples from Oracle.

SELECT JSON_VALUE('{a:100}', '$.a' RETURNING int) AS value;

then the result was null.

But it is wrong, because it should to raise a exception, because this json is broken on Postgres (Postgres requires quoted attribute names)

json_query has same problem

postgres=# SELECT JSON_QUERY('{a:100, b:200, c:300}', '$') AS value;
┌───────┐
│ value │
╞═══════╡
│ ∅     │
└───────┘
(1 row)

It should to check if input is correct json

By the standard, it is implementation-defined whether JSON parsing errors 
should be caught by ON ERROR clause.

SQL/JSON query functions use "JSON API common syntax" which is a combination 
of JSON context item and JSON path.  It passes context item to JSON path 
engine with ALREADY PARSED flag set to False.  ALREADY PARSED flag can enable 
special parsing rules.

Corresponding quotes from the standard:

10.14 <JSON API common syntax> <JSON API common syntax> (    Parameter: "JSON API COMMON SYNTAX" ) Returns: "STATUS" and "SQL/JSON SEQUENCE"

General Rules:
...
3) General Rules of Subclause 9.39, "SQL/JSON path language: syntax and   semantics", are applied with P as PATH SPECIFICATION, C as CONTEXT ITEM,  False as ALREADY PARSED, and PC as PASSING CLAUSE; let ST be the STATUS   and let SEQ be the SQL/JSON SEQUENCE returned from the application of   those General Rules.

9.39 SQL/JSON path language: syntax and semantics

"SQL/JSON path language: syntax and semantics" [General Rules] ( Parameter: "PATH SPECIFICATION", Parameter: "CONTEXT ITEM", Parameter: "ALREADY PARSED", Parameter: "PASSING CLAUSE"
) Returns: "STATUS" and "SQL/JSON SEQUENCE"

General Rules:
...

4) If ALREADY PARSED is False, then it is implementation-defined whether the   following rules are applied: a) The General Rules of Subclause 9.36, "Parsing JSON text", are applied with    JT as JSON TEXT, an implementation-defined <JSON key uniqueness constraint>     as UNIQUENESS CONSTRAINT, and FO as FORMAT OPTION; let ST be the STATUS and    let CISJI be the SQL/JSON ITEM returned from the application of those     General Rules. b) If ST is not successful completion, then ST is returned as the STATUS of    this application of these General Rules, and no further General Rules of     this Subclause are applied.


I decided to apply this rules, so the parsing errors are caught now by ON ERROR 
(NULL ON ERROR is by default).

postgres=# SELECT JSON_VALUE('error', '$' ERROR ON ERROR);
ERROR:  invalid input syntax for type json
DETAIL:  Token "error" is invalid.
CONTEXT:  JSON data, line 1: error

I'm not sure if it would be better to add an implicit cast to json type that
will be executed before so that parsing errors can no longer be caught.
But implicit casting can simplify a bit execution of SQL/JSON query functions.


I have checked error handling in JSON parsing in Oracle 18c/19c, and it behaves
like our current implementation.  But Oracle seems to do JSON parsing on demand:

Oracle19c> SELECT JSON_VALUE('{a:1 error, b:2}', '$.a' ERROR ON ERROR) FROM dual;
1

Oracle19c> SELECT JSON_VALUE('{a:1 error, b:2}', '$.b' ERROR ON ERROR) FROM dual;
ORA-40441: JSON syntax error

Oracle19c> SELECT JSON_VALUE('{a:1 error, b:2}', '$.b') FROM dual;
NULL

Attached 42th version of the patches.

On 18. 1. 2020 v 18:46 Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:

Attached 41th version of the patches.

Changes since previous version:* Enabled DEFAULT clause for ON ERROR/ON EMPTY behaviors in JSON_QUERY()* Added RETURNING clause to JSON_EXISTS() ("side effect" of implementation   EXISTS PATH columns in JSON_TABLE)* ARRAY in EMPTY ARRAY ON ERROR clause is optional now for better Oracle   compatibility

On 17.01.2020 9:54, Pavel Stehule wrote:

I tested cumulative patch - sent in json_table patch.

I almost satisfied by quality of this patch. There is very good conformance with standard and with Oracle. Unfortunately MySQL in this part of JSON support is not compatible.

I found one issue, when I tested some examples from Oracle.

SELECT JSON_VALUE('{a:100}', '$.a' RETURNING int) AS value;

then the result was null.

But it is wrong, because it should to raise a exception, because this json is broken on Postgres (Postgres requires quoted attribute names)

json_query has same problem

postgres=# SELECT JSON_QUERY('{a:100, b:200, c:300}', '$') AS value;
┌───────┐
│ value │
╞═══════╡
│ ∅     │
└───────┘
(1 row)

It should to check if input is correct json

By the standard, it is implementation-defined whether JSON parsing errors 
should be caught by ON ERROR clause.

SQL/JSON query functions use "JSON API common syntax" which is a combination 
of JSON context item and JSON path.  It passes context item to JSON path 
engine with ALREADY PARSED flag set to False.  ALREADY PARSED flag can enable 
special parsing rules.

Corresponding quotes from the standard:

10.14 <JSON API common syntax> <JSON API common syntax> (    Parameter: "JSON API COMMON SYNTAX" ) Returns: "STATUS" and "SQL/JSON SEQUENCE"

General Rules:
...
3) General Rules of Subclause 9.39, "SQL/JSON path language: syntax and   semantics", are applied with P as PATH SPECIFICATION, C as CONTEXT ITEM,  False as ALREADY PARSED, and PC as PASSING CLAUSE; let ST be the STATUS   and let SEQ be the SQL/JSON SEQUENCE returned from the application of   those General Rules.

9.39 SQL/JSON path language: syntax and semantics

"SQL/JSON path language: syntax and semantics" [General Rules] ( Parameter: "PATH SPECIFICATION", Parameter: "CONTEXT ITEM", Parameter: "ALREADY PARSED", Parameter: "PASSING CLAUSE"
) Returns: "STATUS" and "SQL/JSON SEQUENCE"

General Rules:
...

4) If ALREADY PARSED is False, then it is implementation-defined whether the   following rules are applied: a) The General Rules of Subclause 9.36, "Parsing JSON text", are applied with    JT as JSON TEXT, an implementation-defined <JSON key uniqueness constraint>     as UNIQUENESS CONSTRAINT, and FO as FORMAT OPTION; let ST be the STATUS and    let CISJI be the SQL/JSON ITEM returned from the application of those     General Rules. b) If ST is not successful completion, then ST is returned as the STATUS of    this application of these General Rules, and no further General Rules of     this Subclause are applied.


I decided to apply this rules, so the parsing errors are caught now by ON ERROR 
(NULL ON ERROR is by default).

postgres=# SELECT JSON_VALUE('error', '$' ERROR ON ERROR);
ERROR:  invalid input syntax for type json
DETAIL:  Token "error" is invalid.
CONTEXT:  JSON data, line 1: error

I'm not sure if it would be better to add an implicit cast to json type that
will be executed before so that parsing errors can no longer be caught.
But implicit casting can simplify a bit execution of SQL/JSON query functions.


I have checked error handling in JSON parsing in Oracle 18c/19c, and it behaves
like our current implementation.  But Oracle seems to do JSON parsing on demand:

Oracle19c> SELECT JSON_VALUE('{a:1 error, b:2}', '$.a' ERROR ON ERROR) FROM dual;
1

Oracle19c> SELECT JSON_VALUE('{a:1 error, b:2}', '$.b' ERROR ON ERROR) FROM dual;
ORA-40441: JSON syntax error

Oracle19c> SELECT JSON_VALUE('{a:1 error, b:2}', '$.b') FROM dual;
NULL

Everywhere I don't like default masking error. I think so can be very confusing to get NULL
(by default) instead a error of broken format.

I vote for check of input is correct JSON, and if it, then start processing. Else to raise a error.

More - our JSON Parser is different than Oracle's JSON parser. And if somebody will run Oracle's JSONs,
then he get some result on Oracle. But on Postgres, same JSON can be invalid, and he get NULL.

The raising some errors looks like only one safe variant.

I have removed handling of parsing errors in SQL/JSON functions and JSON_TABLE.
Now, FORMAT JSON expressions (implicit or explicit) are simply transformed into
ordinary casts to json type, and these casts are executed before execution of
SQL/JSON functions.  Previously, separate expression was created for such casts,
and it was executed in the separate subtransaction in ExecEvalJsonExpr().  So,
this change also simplifies the code a bit.

On 2020-03-02 23:33, Nikita Glukhov wrote:
> Attached 42th version of the patches.

> v1-0001-Add-jsonpath-pg-modifier-for-enabling-extensions.patch
> v1-0002-Add-raw-jbvArray-and-jbvObject-support-to-jsonpat.patch
> v1-0003-Add-jsonpath-sequence-constructors.patch
> v1-0004-Add-jsonpath-array-constructors.patch
> v1-0005-Add-jsonpath-object-constructors.patch
> v1-0006-Add-jsonpath-object-subscripting.patch

I can't get these to apply against master.


$ patch --dry-run -b -l -F 15 -p 1 < 
0001-Jsonpath-support-for-json-v42.patch
can't find file to patch at input line 38

(tried  -p o and -p 1 -- am I doing it wrong?)


Maybe it needs something else applied first?


Erik Rijkers

On 03.03.2020 2:12, Erik Rijkers wrote:

> On 2020-03-02 23:33, Nikita Glukhov wrote:
>> Attached 42th version of the patches.
>
>> v1-0001-Add-jsonpath-pg-modifier-for-enabling-extensions.patch
>> v1-0002-Add-raw-jbvArray-and-jbvObject-support-to-jsonpat.patch
>> v1-0003-Add-jsonpath-sequence-constructors.patch
>> v1-0004-Add-jsonpath-array-constructors.patch
>> v1-0005-Add-jsonpath-object-constructors.patch
>> v1-0006-Add-jsonpath-object-subscripting.patch
>
> I can't get these to apply against master.
>
>
> $ patch --dry-run -b -l -F 15 -p 1 < 
> 0001-Jsonpath-support-for-json-v42.patch
> can't find file to patch at input line 38
>
> (tried  -p o and -p 1 -- am I doing it wrong?)
>
>
> Maybe it needs something else applied first?
>
>
> Erik Rijkers

Sorry, the wrong patch set was attached.

Patches can be applied the following command (which also creates a commit):
$ git am 0001-Jsonpath-support-for-json-v42.patch

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

On 2020-03-03 00:24, Nikita Glukhov wrote:
> On 03.03.2020 2:12, Erik Rijkers wrote:
> 
>> On 2020-03-02 23:33, Nikita Glukhov wrote:
>>> Attached 42th version of the patches.


20200302/0001-Jsonpath-support-for-json-v42.patch  +
20200302/0002-Add-common-SQL_JSON-clauses-v42.patch+
20200302/0003-Add-invisible-coercion-form-v42.patch+
20200302/0004-Add-function-formats-v42.patch       +
20200302/0005-SQLJSON-constructors-v42.patch       +
20200302/0006-IS-JSON-predicate-v42.patch          +
20200302/0007-SQLJSON-query-functions-v42.patch    +

Thanks -- those applied fine.


Compiling, I get this error from the pg_stat_statements module (in 
contrib):

-- [2020.03.03 02:22:20 json_functions/0] make contrib
pg_stat_statements.c: In function ‘JumbleExpr’:
pg_stat_statements.c:2933:29: error: ‘JsonExpr’ {aka ‘struct JsonExpr’} 
has no member named ‘raw_expr’
  2933 |     JumbleExpr(jstate, jexpr->raw_expr);
       |                             ^~
make[1]: *** [pg_stat_statements.o] Error 1
make: *** [all-pg_stat_statements-recurse] Error 2
-- contrib make returned 2 - abort
../../src/Makefile.global:919: recipe for target 'pg_stat_statements.o' 
failed
Makefile:93: recipe for target 'all-pg_stat_statements-recurse' failed
pg_stat_statements.c: In function ‘JumbleExpr’:
pg_stat_statements.c:2933:29: error: ‘JsonExpr’ {aka ‘struct JsonExpr’} 
has no member named ‘raw_expr’
  2933 |     JumbleExpr(jstate, jexpr->raw_expr);
       |                             ^~
make[1]: *** [pg_stat_statements.o] Error 1
make: *** [install-pg_stat_statements-recurse] Error 2
-- contrib make install returned 2 - abort


(so I've edited out pg_stat_statements from the contrib/Makefile and 
compiled a working server for further testing.)


Thanks,

Erik Rijkers

Attached 43rd version of the patches.

The previous patch #4 ("Add function formats") was removed.
Instead, introduced new executor node JsonCtorExpr which is used to wrap
SQL/JSON constructor function calls (FuncExpr, Aggref, WindowFunc).

Also, JsonIsPredicate node began to be used as executor node.
This helped to remove unnecessary json[b]_is_valid() user functions.

On 06.03.2020 11:16, Pavel Stehule wrote:

make check fails

but probably it is only forgotten actualization

Fixed.

Attached 44th version of the patches.

On 12.03.2020 00:09 Nikita Glukhov wrote:

Attached 43rd version of the patches.

The previous patch #4 ("Add function formats") was removed.
Instead, introduced new executor node JsonCtorExpr which is used to wrap
SQL/JSON constructor function calls (FuncExpr, Aggref, WindowFunc).

Also, JsonIsPredicate node began to be used as executor node.
This helped to remove unnecessary json[b]_is_valid() user functions.

It looks very well - all tests passed, code looks well.

Now, when there are special nodes, then the introduction of COERCE_INTERNAL_CAST is not necessary, and it is only my one and last objection again this patch's set.

I have removed patch #3 with COERCE_INTERNAL_CAST too.

Coercions in JsonValueExpr in JsonCtorExpr, which were previously hidden with
COERCE_INTERNAL_CAST and which were outputted as RETURNING or FORMAT JSON
clauses, now moved into separate expressions.


User functions json[b]_build_object_ext() and json[b]_build_array_ext() also 
can be easily removed.   But it seems harder to remove new aggregate functions 
json[b]_objectagg() and json[b]_agg_strict(), because they can't be called 
directly from JsonCtorExpr node.


The support for json type in jsonpath also seems to be immature, so I will try 
to remove it in the next iteration.

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

Attached 45th version of the patches.

Nodes JsonFormat, JsonReturning, JsonPassing, JsonBehavior were fixed.

út 17. 3. 2020 v 1:55 odesílatel Nikita Glukhov <n.gluhov@postgrespro.ru> napsal:

Attached 44th version of the patches.

On 12.03.2020 16:41, Pavel Stehule wrote:

On 12.03.2020 00:09 Nikita Glukhov wrote:

Attached 43rd version of the patches.

The previous patch #4 ("Add function formats") was removed.
Instead, introduced new executor node JsonCtorExpr which is used to wrap
SQL/JSON constructor function calls (FuncExpr, Aggref, WindowFunc).

Also, JsonIsPredicate node began to be used as executor node.
This helped to remove unnecessary json[b]_is_valid() user functions.

It looks very well - all tests passed, code looks well.

Now, when there are special nodes, then the introduction of COERCE_INTERNAL_CAST is not necessary, and it is only my one and last objection again this patch's set.

I have removed patch #3 with COERCE_INTERNAL_CAST too.

Coercions in JsonValueExpr in JsonCtorExpr, which were previously hidden with
COERCE_INTERNAL_CAST and which were outputted as RETURNING or FORMAT JSON
clauses, now moved into separate expressions.

I am looking on the code, and although the code is correct it doesn't look well (consistently with other node types).

I think so JsonFormat and JsonReturning should be node types, not just structs. If these types will be nodes, then you can simplify _readJsonExpr and all node operations on this node.

 

JsonFormat and JsonReturning was transformed into nodes, and not included 
directly into other nodes now.

User functions json[b]_build_object_ext() and json[b]_build_array_ext() also 
can be easily removed.   But it seems harder to remove new aggregate functions 
json[b]_objectagg() and json[b]_agg_strict(), because they can't be called 
directly from JsonCtorExpr node.

I don't see reasons for another reduction now. Can be great if you can finalize work what you plan for pg13.

+<->READ_ENUM_FIELD(on_error.btype, JsonBehaviorType);
+<->READ_NODE_FIELD(on_error.default_expr);
+<->READ_ENUM_FIELD(on_empty.btype, JsonBehaviorType);
+<->READ_NODE_FIELD(on_empty.default_expr);

JsonBehavior is node type. Then why you don't write just

READ_NODE_FIELD(on_error);

READ_NODE_FIELD(on_empty)

??

JsonBehavior now used in JsonExpr as a pointer to node.

And maybe the code can be better if you don't use JsonPassing struct (or use it only inside gram.y) and pass just List *names, List *values.

Nodes should to contains another nodes or scalar types. Using structs (that are not nodes)  inside doesn't look consistently.

JsonPassing was replaced with two Lists.

I found some not finished code in 0003 patch

+
+json_name_and_value:
+/* TODO
+<-><--><-->KEY c_expr VALUE_P json_value_expr %prec POSTFIXOP
+<-><--><--><-->{ $$ = makeJsonKeyValue($2, $4); }
+<-><--><-->|
+*/

This is unsupported variant of standard syntax, because it seems to lead 
to unresolvable conflicts.  The only supports syntax is:

JSON_OBJECT(key : value)
JSON_OBJECT(key VALUE value)

The support for json type in jsonpath also seems to be immature, so I will try 
to remove it in the next iteration.

What do you think? This patch is little bit off topic, so if you don't need it, then can be removed. Is there some dependency for "jsontable" ?

There is a dependency in SQL/JSON query functions executor, because executor 
uses new structure JsonItem instead of plain JsonbValue.  I will try to
preserve refactoring with JsonItem introduction, but remove json support.
If it will be still unacceptable, I will try to completely remove patch #1.

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

Attached 46th version of the patches.

čt 19. 3. 2020 v 23:57 odesílatel Nikita Glukhov <n.gluhov@postgrespro.ru> napsal:

Attached 45th version of the patches.

Nodes JsonFormat, JsonReturning, JsonPassing, JsonBehavior were fixed.

On 17.03.2020 21:35, Pavel Stehule wrote:

út 17. 3. 2020 v 1:55 odesílatel Nikita Glukhov <n.gluhov@postgrespro.ru> napsal:

Attached 44th version of the patches.

On 12.03.2020 16:41, Pavel Stehule wrote:

On 12.03.2020 00:09 Nikita Glukhov wrote:

Attached 43rd version of the patches.

The previous patch #4 ("Add function formats") was removed.
Instead, introduced new executor node JsonCtorExpr which is used to wrap
SQL/JSON constructor function calls (FuncExpr, Aggref, WindowFunc).

Also, JsonIsPredicate node began to be used as executor node.
This helped to remove unnecessary json[b]_is_valid() user functions.

It looks very well - all tests passed, code looks well.

Now, when there are special nodes, then the introduction of COERCE_INTERNAL_CAST is not necessary, and it is only my one and last objection again this patch's set.

I have removed patch #3 with COERCE_INTERNAL_CAST too.

Coercions in JsonValueExpr in JsonCtorExpr, which were previously hidden with
COERCE_INTERNAL_CAST and which were outputted as RETURNING or FORMAT JSON
clauses, now moved into separate expressions.

I am looking on the code, and although the code is correct it doesn't look well (consistently with other node types).

I think so JsonFormat and JsonReturning should be node types, not just structs. If these types will be nodes, then you can simplify _readJsonExpr and all node operations on this node.

 

JsonFormat and JsonReturning was transformed into nodes, and not included 
directly into other nodes now.

User functions json[b]_build_object_ext() and json[b]_build_array_ext() also 
can be easily removed.   But it seems harder to remove new aggregate functions 
json[b]_objectagg() and json[b]_agg_strict(), because they can't be called 
directly from JsonCtorExpr node.

I don't see reasons for another reduction now. Can be great if you can finalize work what you plan for pg13.

I have removed json[b]_build_object_ext() and json[b]_build_array_ext().

But json[b]_objectagg() and json[b]_agg_strict() are still present.
It seems that removing them requires majors refactoring of the execution
of Aggref and WindowFunc nodes.

+<->READ_ENUM_FIELD(on_error.btype, JsonBehaviorType);
+<->READ_NODE_FIELD(on_error.default_expr);
+<->READ_ENUM_FIELD(on_empty.btype, JsonBehaviorType);
+<->READ_NODE_FIELD(on_empty.default_expr);

JsonBehavior is node type. Then why you don't write just

READ_NODE_FIELD(on_error);

READ_NODE_FIELD(on_empty)

??

JsonBehavior now used in JsonExpr as a pointer to node.

And maybe the code can be better if you don't use JsonPassing struct (or use it only inside gram.y) and pass just List *names, List *values.

Nodes should to contains another nodes or scalar types. Using structs (that are not nodes)  inside doesn't look consistently.

JsonPassing was replaced with two Lists.

I found some not finished code in 0003 patch

+
+json_name_and_value:
+/* TODO
+<-><--><-->KEY c_expr VALUE_P json_value_expr %prec POSTFIXOP
+<-><--><--><-->{ $$ = makeJsonKeyValue($2, $4); }
+<-><--><-->|
+*/

This is unsupported variant of standard syntax, because it seems to lead 
to unresolvable conflicts.  The only supports syntax is:

JSON_OBJECT(key : value)
JSON_OBJECT(key VALUE value)

ok. So please change comment from ToDo to this explanation. Maybe note in doc (unimplemented features can be good idea)

Some these unresolvable conflicts are solved with extra parenthesis in Postgres.

 

The support for json type in jsonpath also seems to be immature, so I will try 
to remove it in the next iteration.

What do you think? This patch is little bit off topic, so if you don't need it, then can be removed. Is there some dependency for "jsontable" ?

There is a dependency in SQL/JSON query functions executor, because executor 
uses new structure JsonItem instead of plain JsonbValue.  I will try to
preserve refactoring with JsonItem introduction, but remove json support.
If it will be still unacceptable, I will try to completely remove patch #1.

I have completely removed patch #1. It turned out to be not so difficult.

I have much better feeling from version 45 (now it looks like Postgres C :)). Still there are some small issues.

1. commented code

+json_encoding:
+<-><--><-->name<--><--><--><--><--><--><--><--><-->{ $$ = makeJsonEncoding($1); }
+<->/*
+<-><--><-->| UTF8<><--><--><--><--><--><--><--><-->{ $$ = JS_ENC_UTF8; }
+<-><--><-->| UTF16><--><--><--><--><--><--><--><-->{ $$ = JS_ENC_UTF16; }
+<-><--><-->| UTF32 <--><--><--><--><--><--><--><-->{ $$ = JS_ENC_UTF32; }
+<->*/
+<-><-->;

Fixed.

2. sometimes useless empty rows

                              silent boolean DEFAULT false)
+RETURNS text
+LANGUAGE INTERNAL
+STRICT IMMUTABLE PARALLEL SAFE
+AS 'jsonb_path_query_first_text';
+
+
+

+<-><-->if (!coerced)
+<-><-->{
+
+<-><--><-->/* If coercion failed, use to_json()/to_jsonb() functions. */

Fixed.

Attached 47th version of the patches.

On 21. 3. 2020 v 11:07 Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:

Attached 46th version of the patches.

On 20.03.2020 22:34, Pavel Stehule wrote:

On 19.03.2020 23:57 Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:

Attached 45th version of the patches.

Nodes JsonFormat, JsonReturning, JsonPassing, JsonBehavior were fixed.

On 17.03.2020 21:35, Pavel Stehule wrote:

User functions json[b]_build_object_ext() and json[b]_build_array_ext() also 
can be easily removed.   But it seems harder to remove new aggregate functions 
json[b]_objectagg() and json[b]_agg_strict(), because they can't be called 
directly from JsonCtorExpr node.

I don't see reasons for another reduction now. Can be great if you can finalize work what you plan for pg13.

I have removed json[b]_build_object_ext() and json[b]_build_array_ext().

But json[b]_objectagg() and json[b]_agg_strict() are still present.
It seems that removing them requires majors refactoring of the execution
of Aggref and WindowFunc nodes.

I have replaced aggregate function

json[b]_objectagg(key any, val any, absent_on_null boolean, unique_keys boolean)

with three separate functions:

json[b]_object_agg_strict(any, any) 
json[b]_object_agg_unique(any, any) 
json[b]_object_agg_unique_strict(any, any)


This should be more correct than single aggregate with additional parameters.

On Mon, Mar 23, 2020 at 8:28 PM Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
> Attached 47th version of the patches.
>
> On 21.03.2020 22:38, Pavel Stehule wrote:
>
>
> On 21. 3. 2020 v 11:07 Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
>>
>> Attached 46th version of the patches.
>>
>> On 20.03.2020 22:34, Pavel Stehule wrote:
>>
>>
>> On 19.03.2020 23:57 Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
>>>
>>> Attached 45th version of the patches.
>>>
>>> Nodes JsonFormat, JsonReturning, JsonPassing, JsonBehavior were fixed.
>>>
>>> On 17.03.2020 21:35, Pavel Stehule wrote:
>>>>
>>>> User functions json[b]_build_object_ext() and json[b]_build_array_ext() also
>>>> can be easily removed.   But it seems harder to remove new aggregate functions
>>>> json[b]_objectagg() and json[b]_agg_strict(), because they can't be called
>>>> directly from JsonCtorExpr node.
>>>
>>>
>>> I don't see reasons for another reduction now. Can be great if you can finalize work what you plan for pg13.
>>>
>> I have removed json[b]_build_object_ext() and json[b]_build_array_ext().
>>
>> But json[b]_objectagg() and json[b]_agg_strict() are still present.
>> It seems that removing them requires majors refactoring of the execution
>> of Aggref and WindowFunc nodes.
>
> I have replaced aggregate function
>
> json[b]_objectagg(key any, val any, absent_on_null boolean, unique_keys boolean)
>
> with three separate functions:
>
> json[b]_object_agg_strict(any, any)
> json[b]_object_agg_unique(any, any)
> json[b]_object_agg_unique_strict(any, any)
>
>
> This should be more correct than single aggregate with additional parameters.

I've following notes about this patchset.

1) Uniqueness checks using JsonbUniqueCheckContext and
JsonUniqueCheckContext have quadratic complexity over number of keys.
That doesn't look good especially for jsonb, which anyway sorts object
keys before object serialization.
2) We have two uniqueness checks for json type, which use
JsonbUniqueCheckContext and JsonUniqueState.  JsonUniqueState uses
stack of hashes, while JsonbUniqueCheckContext have just plain array
of keys.  I think we can make JsonUniqueState use single hash, where
object identifies would be part of hash key.  And we should replace
JsonbUniqueCheckContext with JsonUniqueState.  That would eliminate
extra entities and provide reasonable complexity for cases, which now
use JsonbUniqueCheckContext.
3) New SQL/JSON clauses don't use timezone and considered as immutable
assuming all the children are immutable.  Immutability is good, but
ignoring timezone in all the cases is plain wrong.  The first thing we
can do is to use timezone and make SQL/JSON clauses stable.  But that
limits their usage in functional and partial indexes.  I see couple of
things we can do next (one of them or probably both).
3.1) Provide user a way so specify that we should ignore timezone in
particular case (IGNORE TIMEZONE clause or something like that).  Then
SQL/JSON clause will be considered as immutable.
3.2) Automatically detect whether jsonpath might use timezone.  If
jsonpath doesn't use .datetime() method, it doesn't need timezone for
sure.  Also, from the datetime format specifiers we can get that we
don't compare non-timezoned values to timezoned values.  So, if we
detect this jsonpath never uses timezone, we can consider SQL/JSON
clause as immutable.

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

On Mon, Mar 23, 2020 at 08:28:52PM +0300, Nikita Glukhov wrote:
> Attached 47th version of the patches.

The patch checker/cfbot says this doesn't apply ; could you send a rebasified
version ?

-- 
Justin

On 7/5/20 1:29 PM, Justin Pryzby wrote:
> On Mon, Mar 23, 2020 at 08:28:52PM +0300, Nikita Glukhov wrote:
>> Attached 47th version of the patches.
> The patch checker/cfbot says this doesn't apply ; could you send a rebasified
> version ?
>

To keep things moving, I've rebased these patches. However, 1) the docs
patches use <replaceble class="parameter"> in many cases where they
should now just use <parameter> and b) patch 4 fails when run under
force_parallel=regress.


cheers


andrew



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

On 7/14/20 1:00 PM, Andrew Dunstan wrote:
> On 7/5/20 1:29 PM, Justin Pryzby wrote:
>> On Mon, Mar 23, 2020 at 08:28:52PM +0300, Nikita Glukhov wrote:
>>> Attached 47th version of the patches.
>> The patch checker/cfbot says this doesn't apply ; could you send a rebasified
>> version ?
>>
> To keep things moving, I've rebased these patches. However, 1) the docs
> patches use <replaceble class="parameter"> in many cases where they
> should now just use <parameter> and b) patch 4 fails when run under
> force_parallel=regress.
>
>


Turns out these patches also need to get the message on the new way of
writing entries in func.sgml - I'll publish some updates on that in the
next day or so so that "make doc" will succeed.


cheers


andrew

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

Attached 49th version of the patches with two new patches #5 and #6.

On 7/14/20 1:00 PM, Andrew Dunstan wrote:

On 7/5/20 1:29 PM, Justin Pryzby wrote:

On Mon, Mar 23, 2020 at 08:28:52PM +0300, Nikita Glukhov wrote:

Attached 47th version of the patches.

The patch checker/cfbot says this doesn't apply ; could you send a rebasified
version ?

To keep things moving, I've rebased these patches. However, 1) the docs
patches use <replaceble class="parameter"> in many cases where they
should now just use <parameter> 

I haven't touched <replaceable class="parameter"> yet, because I'm not sure
if <replaceable> or <parameter> is correct here at all.

Turns out these patches also need to get the message on the new way of
writing entries in func.sgml - I'll publish some updates on that in the
next day or so so that "make doc" will succeed.

I can do it by myself, but I just need to understand what to fix and how.

and b) patch 4 fails when run under force_parallel=regress.

Fixed parallel-safety check for RETURNING clause of JSON_EXISTS().

On 05.04.2020 19:50, Alexander Korotkov wrote:
> 1) Uniqueness checks using JsonbUniqueCheckContext and
> JsonUniqueCheckContext have quadratic complexity over number of keys.
> That doesn't look good especially for jsonb, which anyway sorts object
> keys before object serialization.
> 2) We have two uniqueness checks for json type, which use
> JsonbUniqueCheckContext and JsonUniqueState.  JsonUniqueState uses
> stack of hashes, while JsonbUniqueCheckContext have just plain array
> of keys.  I think we can make JsonUniqueState use single hash, where
> object identifies would be part of hash key.  And we should replace
> JsonbUniqueCheckContext with JsonUniqueState.  That would eliminate
> extra entities and provide reasonable complexity for cases, which now
> use JsonbUniqueCheckContext.

Unique checks were refactored as Alexander proposed.

> 3) New SQL/JSON clauses don't use timezone and considered as immutable
> assuming all the children are immutable.  Immutability is good, but
> ignoring timezone in all the cases is plain wrong.  The first thing we
> can do is to use timezone and make SQL/JSON clauses stable.  But that
> limits their usage in functional and partial indexes.  I see couple of
> things we can do next (one of them or probably both).
> 3.1) Provide user a way so specify that we should ignore timezone in
> particular case (IGNORE TIMEZONE clause or something like that).  Then
> SQL/JSON clause will be considered as immutable.
> 3.2) Automatically detect whether jsonpath might use timezone.  If
> jsonpath doesn't use .datetime() method, it doesn't need timezone for
> sure.  Also, from the datetime format specifiers we can get that we
> don't compare non-timezoned values to timezoned values.  So, if we
> detect this jsonpath never uses timezone, we can consider SQL/JSON
> clause as immutable.

Implemented second variant with automatic detection.

I also tried to add explicit IGNORE TIMEZONE / IMMUTABLE clauses, but all of
them lead to shift/reduce conflicts that seem not easy to resolve.



Patch #5 implements functions for new JSON type that is expected to appear in
the upcoming SQL/JSON standard:
 - JSON() is for constructing JSON typed values from JSON text.   It is almost equivalent to text::json[b] cast, except that it has additional   ability to specify WITH UNIQUE KEYS constraint.
 - JSON_SCALAR() is for constructing JSON typed values from SQL scalars.   It is equivalent to to_json[b]().
 - JSON_SERIALIZE() is for serializing JSON typed values to character strings.   It is almost equivalent to json[b]::character_type cast, but it also   supports output to bytea.

Upcoming Oracle 20c will have JSON datatype and these functions [1], so we
decided also to implement them for compatibility, despite that they do not make
sense for real PG users.



Patch #6 allows the user to use PG jsonb type as an effective implementation of
SQL JSON type.  By explicitly setting GUC sql_json = jsonb, JSON will be mapped
to jsonb, and JSON TEXT (may be named named differently) will be mapped to json.

This seems to be a hack, but we failed to propose something more simpler.

Example of usage GUC sql_json:

=# CREATE TABLE test1 (js json, jb jsonb, jt json text);
CREATE TABLE

=# \d test1              Table "public.test1" Column | Type  | Collation | Nullable | Default 
--------+-------+-----------+----------+--------- js     | json  |           |          |  jb     | jsonb |           |          |  jt     | json  |           |          | 

=# SET sql_json = jsonb;
SET

=# \d test1                Table "public.test1" Column |   Type    | Collation | Nullable | Default 
--------+-----------+-----------+----------+--------- js     | json text |           |          |  jb     | json      |           |          |  jt     | json text |           |          | 

=# CREATE TABLE test2 (js json, jb jsonb, jt json text);
CREATE TABLE

=# \d test2                Table "public.test2" Column |   Type    | Collation | Nullable | Default 
--------+-----------+-----------+----------+--------- js     | json      |           |          |  jb     | json      |           |          |  jt     | json text |           |          | 

=# SET sql_json = json;
SET
=# \d test2              Table "public.test2" Column | Type  | Collation | Nullable | Default 
--------+-------+-----------+----------+--------- js     | jsonb |           |          |  jb     | jsonb |           |          |  jt     | json  |           |          | 



[1] https://docs.oracle.com/en/database/oracle/oracle-database/20/adjsn/json-in-oracle-database.html#GUID-CBEDC779-39A3-43C9-AF38-861AE3FC0AEC

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

On 7/14/20 9:47 PM, Nikita Glukhov wrote:
> Attached 49th version of the patches with two new patches #5 and #6.
>
> On 15.07.2020 00:09, Andrew Dunstan wrote:
>> On 7/14/20 1:00 PM, Andrew Dunstan wrote:
>>> On 7/5/20 1:29 PM, Justin Pryzby wrote:
>>>> On Mon, Mar 23, 2020 at 08:28:52PM +0300, Nikita Glukhov wrote:
>>>>> Attached 47th version of the patches.
>>>> The patch checker/cfbot says this doesn't apply ; could you send a rebasified
>>>> version ?
>>>>
>>> To keep things moving, I've rebased these patches. However, 1) the docs
>>> patches use <replaceble class="parameter"> in many cases where they
>>> should now just use <parameter> 
> I haven't touched <replaceable class="parameter"> yet, because I'm not sure
> if <replaceable> or <parameter> is correct here at all.



Here's the relevant commit message that explains the policy:


commit 47046763c3
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   Mon May 4 13:48:30 2020 -0400

    Doc: standardize markup a bit more.
   
    We had a mishmash of <replaceable>, <replaceable class="parameter">,
    and <parameter> markup for operator/function arguments.  Use <parameter>
    consistently for things that are in fact names of parameters (including
    OUT parameters), reserving <replaceable> for things that aren't.  The
    latter class includes some made-up-by-the-docs type class names, like
    "numeric_type", as well as placeholders for arguments that don't have
    well-defined types.  Possibly we could do better with those categories
    as well, but for the moment I'm content not to have parameter names
    marked up in different ways in different places.



>
>> Turns out these patches also need to get the message on the new way of
>> writing entries in func.sgml - I'll publish some updates on that in the
>> next day or so so that "make doc" will succeed.
> I can do it by myself, but I just need to understand what to fix and how.



Here's an example that shows how it was transformed for json_agg:


-     <row>
-      <entry>
-       <indexterm>
-        <primary>json_agg</primary>
-       </indexterm>
-       <function>json_agg(<replaceable
class="parameter">expression</replaceable>)</function>
-      </entry>
-      <entry>
-       <type>any</type>
-      </entry>
-      <entry>
-       <type>json</type>
-      </entry>
-      <entry>No</entry>
-      <entry>aggregates values, including nulls, as a JSON array</entry>
-     </row>
+      <row>
+       <entry role="func_table_entry"><para role="func_signature">
+        <indexterm>
+         <primary>json_agg</primary>
+        </indexterm>
+        <function>json_agg</function> ( <type>anyelement</type> )
+        <returnvalue>json</returnvalue>
+       </para>
+       <para role="func_signature">
+        <indexterm>
+         <primary>jsonb_agg</primary>
+        </indexterm>
+        <function>jsonb_agg</function> ( <type>anyelement</type> )
+        <returnvalue>jsonb</returnvalue>
+       </para>
+       <para>
+        Collects all the input values, including nulls, into a JSON array.
+        Values are converted to JSON as per <function>to_json</function>
+        or <function>to_jsonb</function>.
+       </para></entry>
+       <entry>No</entry>
+      </row>
 



>>> and b) patch 4 fails when run under force_parallel=regress.
> Fixed parallel-safety check for RETURNING clause of JSON_EXISTS().
>
> On 05.04.2020 19:50, Alexander Korotkov wrote:
> > 1) Uniqueness checks using JsonbUniqueCheckContext and
> > JsonUniqueCheckContext have quadratic complexity over number of keys.
> > That doesn't look good especially for jsonb, which anyway sorts object
> > keys before object serialization.
> > 2) We have two uniqueness checks for json type, which use
> > JsonbUniqueCheckContext and JsonUniqueState.  JsonUniqueState uses
> > stack of hashes, while JsonbUniqueCheckContext have just plain array
> > of keys.  I think we can make JsonUniqueState use single hash, where
> > object identifies would be part of hash key.  And we should replace
> > JsonbUniqueCheckContext with JsonUniqueState.  That would eliminate
> > extra entities and provide reasonable complexity for cases, which now
> > use JsonbUniqueCheckContext.
>
> Unique checks were refactored as Alexander proposed.
>
> > 3) New SQL/JSON clauses don't use timezone and considered as immutable
> > assuming all the children are immutable.  Immutability is good, but
> > ignoring timezone in all the cases is plain wrong.  The first thing we
> > can do is to use timezone and make SQL/JSON clauses stable.  But that
> > limits their usage in functional and partial indexes.  I see couple of
> > things we can do next (one of them or probably both).
> > 3.1) Provide user a way so specify that we should ignore timezone in
> > particular case (IGNORE TIMEZONE clause or something like that).  Then
> > SQL/JSON clause will be considered as immutable.
> > 3.2) Automatically detect whether jsonpath might use timezone.  If
> > jsonpath doesn't use .datetime() method, it doesn't need timezone for
> > sure.  Also, from the datetime format specifiers we can get that we
> > don't compare non-timezoned values to timezoned values.  So, if we
> > detect this jsonpath never uses timezone, we can consider SQL/JSON
> > clause as immutable.
>
> Implemented second variant with automatic detection.
>
> I also tried to add explicit IGNORE TIMEZONE / IMMUTABLE clauses, but all of
> them lead to shift/reduce conflicts that seem not easy to resolve.




Not sure if this helps:


https://wiki.postgresql.org/wiki/Debugging_the_PostgreSQL_grammar_(Bison)



>
>
>
> Patch #5 implements functions for new JSON type that is expected to appear in
> the upcoming SQL/JSON standard:
>
>  - JSON() is for constructing JSON typed values from JSON text.
>    It is almost equivalent to text::json[b] cast, except that it has additional
>    ability to specify WITH UNIQUE KEYS constraint.
>
>  - JSON_SCALAR() is for constructing JSON typed values from SQL scalars.
>    It is equivalent to to_json[b]().
>
>  - JSON_SERIALIZE() is for serializing JSON typed values to character strings.
>    It is almost equivalent to json[b]::character_type cast, but it also
>    supports output to bytea.
>
> Upcoming Oracle 20c will have JSON datatype and these functions [1], so we
> decided also to implement them for compatibility, despite that they do not make
> sense for real PG users.
>


Are these functions in the standard, or are they Oracle extensions? If
the latter maybe they belong in an options extension.



>
> Patch #6 allows the user to use PG jsonb type as an effective implementation of
> SQL JSON type.  By explicitly setting GUC sql_json = jsonb, JSON will be mapped
> to jsonb, and JSON TEXT (may be named named differently) will be mapped to json.
>
> This seems to be a hack, but we failed to propose something more simpler.



What is going to be the effect of that on things like index expressions?
This strikes me at first glance as something of a potential footgun, but
maybe I'm being overcautious.


cheers


andrew




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

Attached 50th version of the patches. Only the documentation was changed
since the previous version.

On 15.07.2020 14:50, Andrew Dunstan wrote:

On 7/14/20 9:47 PM, Nikita Glukhov wrote:

On 15.07.2020 00:09, Andrew Dunstan wrote:

On 7/14/20 1:00 PM, Andrew Dunstan wrote:

To keep things moving, I've rebased these patches. However, 1) the docs
patches use <replaceble class="parameter"> in many cases where they
should now just use <parameter> 

I haven't touched <replaceable class="parameter"> yet, because I'm not sure
if <replaceable> or <parameter> is correct here at all.

Here's the relevant commit message that explains the policy:

commit 47046763c3
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   Mon May 4 13:48:30 2020 -0400

    Doc: standardize markup a bit more.
   
    We had a mishmash of <replaceable>, <replaceable class="parameter">,
    and <parameter> markup for operator/function arguments.  Use <parameter>
    consistently for things that are in fact names of parameters (including
    OUT parameters), reserving <replaceable> for things that aren't.  The
    latter class includes some made-up-by-the-docs type class names, like
    "numeric_type", as well as placeholders for arguments that don't have
    well-defined types.  Possibly we could do better with those categories
    as well, but for the moment I'm content not to have parameter names
    marked up in different ways in different places.

Turns out these patches also need to get the message on the new way of
writing entries in func.sgml - I'll publish some updates on that in the
next day or so so that "make doc" will succeed.

I can do it by myself, but I just need to understand what to fix and how.

Ok, I replaced some <replaceable> with <parameter> in SQL/JSON constructors.
Also I replaced all '[]' with <optional>.

Patch #5 implements functions for new JSON type that is expected to appear in
the upcoming SQL/JSON standard:
 - JSON() is for constructing JSON typed values from JSON text.   It is almost equivalent to text::json[b] cast, except that it has additional   ability to specify WITH UNIQUE KEYS constraint.
 - JSON_SCALAR() is for constructing JSON typed values from SQL scalars.   It is equivalent to to_json[b]().
 - JSON_SERIALIZE() is for serializing JSON typed values to character strings.   It is almost equivalent to json[b]::character_type cast, but it also   supports output to bytea.

Upcoming Oracle 20c will have JSON datatype and these functions [1], so we
decided also to implement them for compatibility, despite that they do not make
sense for real PG users.

Are these functions in the standard, or are they Oracle extensions? If
the latter maybe they belong in an options extension.

The new SQL type JSON and these functions are in to the new standard SQL/JSON 2.0. 
It is at the proposal stage now, but Oracle 20c has already implemented it.

The document is not publicly available, so Oleg should have sent it to you in a 
private message.

Patch #6 allows the user to use PG jsonb type as an effective implementation of
SQL JSON type.  By explicitly setting GUC sql_json = jsonb, JSON will be mapped
to jsonb, and JSON TEXT (may be named named differently) will be mapped to json.

This seems to be a hack, but we failed to propose something more simpler.

What is going to be the effect of that on things like index expressions?
This strikes me at first glance as something of a potential footgun, but
maybe I'm being overcautious.

This allows users of 'sql_json=jsonb' to create GIN indexes on SQL type JSON
(but such creation indexes are still not SQL standard conforming). Maybe I do 
not correctly understand the question or the consequences of type name 
rewriting.

The type names are rewritten on the input at the initial parsing stage and on 
the output in format_type_be(), like it is done for "timestamp with timezone" =>
timestamptz, integer => int4.  Internal representation of query expressions 
remains the same.  Affected only representation of JSON types to/from user.

On 7/17/20 4:26 PM, Nikita Glukhov wrote:
>
>>> Patch #5 implements functions for new JSON type that is expected to appear in
>>> the upcoming SQL/JSON standard:
>>>
>>>  - JSON() is for constructing JSON typed values from JSON text.
>>>    It is almost equivalent to text::json[b] cast, except that it has additional
>>>    ability to specify WITH UNIQUE KEYS constraint.
>>>
>>>  - JSON_SCALAR() is for constructing JSON typed values from SQL scalars.
>>>    It is equivalent to to_json[b]().
>>>
>>>  - JSON_SERIALIZE() is for serializing JSON typed values to character strings.
>>>    It is almost equivalent to json[b]::character_type cast, but it also
>>>    supports output to bytea.
>>>
>>> Upcoming Oracle 20c will have JSON datatype and these functions [1], so we
>>> decided also to implement them for compatibility, despite that they do not make
>>> sense for real PG users.
>> Are these functions in the standard, or are they Oracle extensions? If
>> the latter maybe they belong in an options extension.
> The new SQL type JSON and these functions are in to the new standard SQL/JSON 2.0. 
> It is at the proposal stage now, but Oracle 20c has already implemented it.
>
> The document is not publicly available, so Oleg should have sent it to you in a 
> private message.
>
>>> Patch #6 allows the user to use PG jsonb type as an effective implementation of
>>> SQL JSON type.  By explicitly setting GUC sql_json = jsonb, JSON will be mapped
>>> to jsonb, and JSON TEXT (may be named named differently) will be mapped to json.
>>>
>>> This seems to be a hack, but we failed to propose something more simpler.
>> What is going to be the effect of that on things like index expressions?
>> This strikes me at first glance as something of a potential footgun, but
>> maybe I'm being overcautious.
> This allows users of 'sql_json=jsonb' to create GIN indexes on SQL type JSON
> (but such creation indexes are still not SQL standard conforming). Maybe I do 
> not correctly understand the question or the consequences of type name 
> rewriting.
>
> The type names are rewritten on the input at the initial parsing stage and on 
> the output in format_type_be(), like it is done for "timestamp with timezone" =>
> timestamptz, integer => int4.  Internal representation of query expressions 
> remains the same.  Affected only representation of JSON types to/from user.
>


I think patches 5 and 6 need to be submitted to the next commitfest,
This is far too much scope creep to be snuck in under the current CF item.


I'll look at patches 1-4.


cheers


andrew


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

On Sat, Jul 18, 2020 at 09:24:11AM -0400, Andrew Dunstan wrote:
> I think patches 5 and 6 need to be submitted to the next commitfest,
> This is far too much scope creep to be snuck in under the current CF item.
>
> I'll look at patches 1-4.

Even with that, the patch set has been waiting on author for the last
six weeks, so I am marking it as RwF for now.  Please feel free to
resubmit.
--
Michael

On Fri, 17 Jul 2020 at 21:26, Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
>
> Attached 50th version of the patches. Only the documentation was changed
> since the previous version.

I can imagine the effort required to get to v50, so I salute your efforts.

The document for SQL Standard has now been published as CD
9075-2-Foundation (Thanks Peter).

That gives us a clearer picture of what is being voted on and should
allow Nikita to complete his work.

I suggest we move forwards on this now, but if anyone objects to
including this in PG14 in favour of waiting for the vote, please say
so clearly so we can skip to PG15.

-- 
Simon Riggs                http://www.EnterpriseDB.com/

On Tue, Dec 15, 2020 at 8:01 PM Simon Riggs <simon@2ndquadrant.com> wrote:
>
> On Fri, 17 Jul 2020 at 21:26, Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
> >
> > Attached 50th version of the patches. Only the documentation was changed
> > since the previous version.
>
> I can imagine the effort required to get to v50, so I salute your efforts.
>
> The document for SQL Standard has now been published as CD
> 9075-2-Foundation (Thanks Peter).

SQL-2016 is already 4 years old and this what we are using for our development.
What is CD 9075-2-Foundation ?


>
> That gives us a clearer picture of what is being voted on and should
> allow Nikita to complete his work.
>
> I suggest we move forwards on this now, but if anyone objects to
> including this in PG14 in favour of waiting for the vote, please say
> so clearly so we can skip to PG15.

There is upcoming JSON v2. SQL Standard, which  specifies JSON data type,
that mean we have to protect our users, or SQL-compliant applications will be
slow and developers will be disappointed.  I discussed all these in my talk
at Postgres Build
http://www.sai.msu.su/~megera/postgres/talks/json-build-2020.pdf
Andrew Dunstan has volunteered to work with us on completing SQL/JSON standard,
but ...

I think we need SQL/JSON functions in PG14 and  SQL/JSON compatibility mode for
json/jsonb.


>
> --
> Simon Riggs                http://www.EnterpriseDB.com/



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

On Tue, Dec 15, 2020 at 8:37 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
>
> út 15. 12. 2020 v 18:00 odesílatel Simon Riggs <simon@2ndquadrant.com> napsal:
>>
>> On Fri, 17 Jul 2020 at 21:26, Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
>> >
>> > Attached 50th version of the patches. Only the documentation was changed
>> > since the previous version.
>>
>> I can imagine the effort required to get to v50, so I salute your efforts.
>>
>> The document for SQL Standard has now been published as CD
>> 9075-2-Foundation (Thanks Peter).
>>
>> That gives us a clearer picture of what is being voted on and should
>> allow Nikita to complete his work.
>>
>> I suggest we move forwards on this now, but if anyone objects to
>> including this in PG14 in favour of waiting for the vote, please say
>> so clearly so we can skip to PG15.
>
>
> Maybe this set of patches can be reorganized and divided. Some parts like json generating functions are almost
trivialand without controversions with clean benefits for users. 

I agree with this, most interesting is JSON_TABLE.
>
> The most complexity is related to json_table function. Nikita did a very good job and implemented this function in
maximalconformance with ANSI SQL with a maximal set of features. On second hand it is hard to do review because this
patchis really complex, and a lot of functionality was not implemented elsewhere (so isn't possible to compare
results).I think it should be possible to reduce complexity and divide acceptance of json_table to some steps like
basicfunctionality (on MySQL level), enhanced functionality (on Oracle level), and full functionality (the Postgres
willbe first).  This functionality is interesting and maximal conformity with SQL/JSON is great so I am for merging it.
Butif it will be divided into some chronological steps, then there can be higher probability of merging to upstream in
agood time. 

I think it's shame to look up on MySQL and Oracle, since we have much
better and complete implementation of the Standard.




>
> Regards
>
> Pavel
>
>>
>> --
>> Simon Riggs                http://www.EnterpriseDB.com/



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

On 17.09.2020 08:41, Michael Paquier wrote:

On Sat, Jul 18, 2020 at 09:24:11AM -0400, Andrew Dunstan wrote:

I think patches 5 and 6 need to be submitted to the next commitfest,
This is far too much scope creep to be snuck in under the current CF item.

I'll look at patches 1-4.

Even with that, the patch set has been waiting on author for the last
six weeks, so I am marking it as RwF for now.  Please feel free to
resubmit.

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


There were some shift/reduce conflicts in SQL grammar that have appeared
after "expr AS keyword" refactoring in 06a7c3154f.  I'm not sure if I resolved
them correctly.  JSON TEXT pseudotype, introduced in #0006, caused a lot of
grammar conflicts, so it was replaced with simple explicit pg_catalog.json.

Also new CoercionForm COERCE_SQL_SYNTAX was introduced, and this reminds custom
function formats that I have used in earlier version of the patches for
deparsing of SQL/JSON constructor expressions that were based on raw json[b]
function calls.  These custom function formats were replaced in v43 with
dedicated executor nodes for SQL/JSON constructors.  So, I'm not sure is it
worth to try to replace back nodes with new COERCE_SQL_SYNTAX.

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

Hi, Zhihong.  Thank your for your review.  Attached 52nd version of the 
patches rebased onto master and fixed as you suggested.

For 0001-Common-SQL-JSON-clauses-v51.patch :

+       /*  | implementation_defined_JSON_representation_option (BSON, AVRO etc) */

I don't find implementation_defined_JSON_representation_option in the patchset. Maybe rephrase the above as a comment without implementation_defined_JSON_representation_option ?

Fixed.

For getJsonEncodingConst(), should the method error out for the default case of switch (encoding) ?

Added default case with elog(ERROR).

0002-SQL-JSON-constructors-v51.patch :

+                   Assert(!OidIsValid(collation)); /* result is always an json[b] type */

an json -> a json

Fixed.

+           /* XXX TEXTOID is default by standard */
+           returning->typid = JSONOID;

Comment doesn't seem to match the assignment.

Comment corrected.

For json_object_agg_transfn :

+       if (out->len > 2)
+           appendStringInfoString(out, ", ");

Why length needs to be at least 3 (instead of 2) ?

2 is a length of starting string "{ ".  len > 2 means that we have already 
outputted some fields and we need to output comma delimiter.

Hi,

For ExecEvalJsonExprSubtrans(), if you check !subtrans first,

+       /* No need to use subtransactions. */
+       return func(op, econtext, res, resnull, p, error);

The return statement would allow omitting the else keyword and left-indent the code in the current if block.

"If" statement was refactored as you suggest.

For ExecEvalJsonExpr()

+           *resnull = !DatumGetPointer(res);
+           if (error && *error)
+               return (Datum) 0;

Suppose *resnull is false and *error is true, 0 would be returned with *resnull as false. Should the *resnull be consistent with the actual return value ?

Now *resnull is set to false in case of error.

For ExecEvalJson() :

+       Assert(*op->resnull);
+       *op->resnull = true;

I am not sure of the purpose for the assignment since *op->resnull should be true by the assertion.

Assignment was removed.

For raw_expression_tree_walker :

+               if (walker(jfe->on_empty, context))
+                   return true;

Should the if condition include jfe->on_empty prior to walking ?

Yes, jfe->on_empty like jfe->on_error can be NULL, and NULL check here is
a walker's responsibility.  But in expression_tree_walker() there is a check 
for jfe->on_empty, because only on_empty (not jfe->on_error) can be NULL, and 
we are calling walker() on jfe->on_empty->default_expr, not on jfe->on_empty.

nit: for contain_mutable_functions_walker, if !IsA(jexpr->path_spec, Const) is checked first (and return), the current if block can be left indented.

Code was refactored as you suggested.

For JsonPathDatatypeStatus,

+   jpdsDateTime,               /* unknown datetime type */

Should the enum be named jpdsUnknownDateTime so that its meaning is clear to people reading the code ?

jpdsDateTime was renamed to jpdsUnknownDateTime.

For get_json_behavior(), I wonder if mapping from behavior->btype to the string form would shorten the body of switch statement.

e.g.

char* map[] = {

  " NULL",

  " ERROR",

  " EMPTY",

...

};

"Switch" statement was replaced with array lookup.

On 2021-01-20 03:49, Nikita Glukhov wrote:

> [0001-Add-common-SQL-JSON-clauses-v52.patch.gz]
> [0002-SQL-JSON-constructors-v52.patch.gz]
> [0003-IS-JSON-predicate-v52.patch.gz]
> [0004-SQL-JSON-query-functions-v52.patch.gz]
> [0005-SQL-JSON-functions-for-json-type-v52.patch.gz]
> [0006-GUC-sql_json-v52.patch.gz]

Hi,

I read through the file func.sgml (only that file) and put the 
errors/peculiarities in the attached diff.  (Small stuff; typos really)


Your patch includes a CREATE TABLE my_films + INSERT, to run the 
examples against.  I think this is a great idea and we should do it more 
often.

But, the table has a text-column to contain the subsequently inserted 
json values. The insert runs fine but it turns out that some later 
examples queries only run against a jsonb column.  So I propose to 
change:
   CREATE TABLE my_films (js text);
to:
   CREATE TABLE my_films (js jsonb);

This change is not yet included in the attached file.  An alternative 
would be to cast the text-column in the example queries as js::jsonb


I also noticed that some errors were different in the sgml file than 'in 
the event':


    SELECT JSON_QUERY(js, '$.favorites[*].kind' ERROR ON ERROR) FROM 
my_films_jsonb;
    (table 'my_films_jsonb' is the same as your 'my_films', but with js 
as a jsonb column)

manual says: "ERROR: more than one SQL/JSON item"
  in reality: "ERROR: JSON path expression in JSON_QUERY should return 
singleton item without wrapper"
         and:   "HINT: use WITH WRAPPER clause to wrap SQL/JSON item 
sequence into array"


Thanks,

Erik Rijkers

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

On 3/8/21 1:55 PM, Ibrar Ahmed wrote:
>
>
> On Sat, Jan 23, 2021 at 3:37 PM Erik Rijkers <er@xs4all.nl
> <mailto:er@xs4all.nl>> wrote:
>
>     On 2021-01-20 03:49, Nikita Glukhov wrote:
>
>     > [0001-Add-common-SQL-JSON-clauses-v52.patch.gz]
>     > [0002-SQL-JSON-constructors-v52.patch.gz]
>     > [0003-IS-JSON-predicate-v52.patch.gz]
>     > [0004-SQL-JSON-query-functions-v52.patch.gz]
>     > [0005-SQL-JSON-functions-for-json-type-v52.patch.gz]
>     > [0006-GUC-sql_json-v52.patch.gz]
>
>     Hi,
>
>     I read through the file func.sgml (only that file) and put the
>     errors/peculiarities in the attached diff.  (Small stuff; typos
>     really)
>
>
>     Your patch includes a CREATE TABLE my_films + INSERT, to run the
>     examples against.  I think this is a great idea and we should do
>     it more
>     often.
>
>     But, the table has a text-column to contain the subsequently inserted
>     json values. The insert runs fine but it turns out that some later
>     examples queries only run against a jsonb column.  So I propose to
>     change:
>        CREATE TABLE my_films (js text);
>     to:
>        CREATE TABLE my_films (js jsonb);
>
>     This change is not yet included in the attached file.  An alternative
>     would be to cast the text-column in the example queries as js::jsonb
>
>
>     I also noticed that some errors were different in the sgml file
>     than 'in
>     the event':
>
>
>         SELECT JSON_QUERY(js, '$.favorites[*].kind' ERROR ON ERROR) FROM
>     my_films_jsonb;
>         (table 'my_films_jsonb' is the same as your 'my_films', but
>     with js
>     as a jsonb column)
>
>     manual says: "ERROR: more than one SQL/JSON item"
>       in reality: "ERROR: JSON path expression in JSON_QUERY should
>     return
>     singleton item without wrapper"
>              and:   "HINT: use WITH WRAPPER clause to wrap SQL/JSON item
>     sequence into array"
>
>
>     Thanks,
>
>     Erik Rijkers
>
>     >
>     > --
>     > Nikita Glukhov
>     > Postgres Professional: http://www.postgrespro.com
>     <http://www.postgrespro.com>
>     > The Russian Postgres Company
>
>
> The patch (func.sgml.20210123.diff) does not apply successfully.
>
> http://cfbot.cputube.org/patch_32_2901.log
> <http://cfbot.cputube.org/patch_32_2901.log>
>
> ----
> === Applying patches on top of PostgreSQL commit ID 0ce4cd04da558178b0186057b721c50a00b7a945 ===
> === applying patch ./func.sgml.20210123.diff
> patching file doc/src/sgml/func.sgml
> Hunk #1 FAILED at 16968.
> Hunk #2 FAILED at 17034.
> ...
> Hunk #19 FAILED at 18743.
> 19 out of 19 hunks FAILED -- saving rejects to file doc/src/sgml/func.sgml.rej
> ----
>
> Can we get a rebase? 
>
> I am marking the patch "Waiting on Author".




I've rebased this, and applied some of Erik's changes.


I'll set it back to 'Needs Review' if the cfbot is happy.


cheers


andrew


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

On 3/26/21 4:22 PM, Andrew Dunstan wrote:
> On 3/8/21 1:55 PM, Ibrar Ahmed wrote:
>>
>> On Sat, Jan 23, 2021 at 3:37 PM Erik Rijkers <er@xs4all.nl
>> <mailto:er@xs4all.nl>> wrote:
>>
>>     On 2021-01-20 03:49, Nikita Glukhov wrote:
>>
>>     > [0001-Add-common-SQL-JSON-clauses-v52.patch.gz]
>>     > [0002-SQL-JSON-constructors-v52.patch.gz]
>>     > [0003-IS-JSON-predicate-v52.patch.gz]
>>     > [0004-SQL-JSON-query-functions-v52.patch.gz]
>>     > [0005-SQL-JSON-functions-for-json-type-v52.patch.gz]
>>     > [0006-GUC-sql_json-v52.patch.gz]
>>
>>     Hi,
>>
>>     I read through the file func.sgml (only that file) and put the
>>     errors/peculiarities in the attached diff.  (Small stuff; typos
>>     really)
>>
>>
>>     Your patch includes a CREATE TABLE my_films + INSERT, to run the
>>     examples against.  I think this is a great idea and we should do
>>     it more
>>     often.
>>
>>     But, the table has a text-column to contain the subsequently inserted
>>     json values. The insert runs fine but it turns out that some later
>>     examples queries only run against a jsonb column.  So I propose to
>>     change:
>>        CREATE TABLE my_films (js text);
>>     to:
>>        CREATE TABLE my_films (js jsonb);
>>
>>     This change is not yet included in the attached file.  An alternative
>>     would be to cast the text-column in the example queries as js::jsonb
>>
>>
>>     I also noticed that some errors were different in the sgml file
>>     than 'in
>>     the event':
>>
>>
>>         SELECT JSON_QUERY(js, '$.favorites[*].kind' ERROR ON ERROR) FROM
>>     my_films_jsonb;
>>         (table 'my_films_jsonb' is the same as your 'my_films', but
>>     with js
>>     as a jsonb column)
>>
>>     manual says: "ERROR: more than one SQL/JSON item"
>>       in reality: "ERROR: JSON path expression in JSON_QUERY should
>>     return
>>     singleton item without wrapper"
>>              and:   "HINT: use WITH WRAPPER clause to wrap SQL/JSON item
>>     sequence into array"
>>
>>
>>     Thanks,
>>
>>     Erik Rijkers
>>
>>     >
>>     > --
>>     > Nikita Glukhov
>>     > Postgres Professional: http://www.postgrespro.com
>>     <http://www.postgrespro.com>
>>     > The Russian Postgres Company
>>
>>
>> The patch (func.sgml.20210123.diff) does not apply successfully.
>>
>> http://cfbot.cputube.org/patch_32_2901.log
>> <http://cfbot.cputube.org/patch_32_2901.log>
>>
>> ----
>> === Applying patches on top of PostgreSQL commit ID 0ce4cd04da558178b0186057b721c50a00b7a945 ===
>> === applying patch ./func.sgml.20210123.diff
>> patching file doc/src/sgml/func.sgml
>> Hunk #1 FAILED at 16968.
>> Hunk #2 FAILED at 17034.
>> ...
>> Hunk #19 FAILED at 18743.
>> 19 out of 19 hunks FAILED -- saving rejects to file doc/src/sgml/func.sgml.rej
>> ----
>>
>> Can we get a rebase? 
>>
>> I am marking the patch "Waiting on Author".
>
>
>
> I've rebased this, and applied some of Erik's changes.
>
>
> I'll set it back to 'Needs Review' if the cfbot is happy.


It's not. There are errors  when building with llvm. I'll investigate.


cheers


andrew


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

On 3/26/21 4:49 PM, Andrew Dunstan wrote:
> On 3/26/21 4:22 PM, Andrew Dunstan wrote:
>> On 3/8/21 1:55 PM, Ibrar Ahmed wrote:
>>> On Sat, Jan 23, 2021 at 3:37 PM Erik Rijkers <er@xs4all.nl
>>> <mailto:er@xs4all.nl>> wrote:
>>>
>>>     On 2021-01-20 03:49, Nikita Glukhov wrote:
>>>
>>>     > [0001-Add-common-SQL-JSON-clauses-v52.patch.gz]
>>>     > [0002-SQL-JSON-constructors-v52.patch.gz]
>>>     > [0003-IS-JSON-predicate-v52.patch.gz]
>>>     > [0004-SQL-JSON-query-functions-v52.patch.gz]
>>>     > [0005-SQL-JSON-functions-for-json-type-v52.patch.gz]
>>>     > [0006-GUC-sql_json-v52.patch.gz]
>>>
>>>     Hi,
>>>
>>>     I read through the file func.sgml (only that file) and put the
>>>     errors/peculiarities in the attached diff.  (Small stuff; typos
>>>     really)
>>>
>>>
>>>     Your patch includes a CREATE TABLE my_films + INSERT, to run the
>>>     examples against.  I think this is a great idea and we should do
>>>     it more
>>>     often.
>>>
>>>     But, the table has a text-column to contain the subsequently inserted
>>>     json values. The insert runs fine but it turns out that some later
>>>     examples queries only run against a jsonb column.  So I propose to
>>>     change:
>>>        CREATE TABLE my_films (js text);
>>>     to:
>>>        CREATE TABLE my_films (js jsonb);
>>>
>>>     This change is not yet included in the attached file.  An alternative
>>>     would be to cast the text-column in the example queries as js::jsonb
>>>
>>>
>>>     I also noticed that some errors were different in the sgml file
>>>     than 'in
>>>     the event':
>>>
>>>
>>>         SELECT JSON_QUERY(js, '$.favorites[*].kind' ERROR ON ERROR) FROM
>>>     my_films_jsonb;
>>>         (table 'my_films_jsonb' is the same as your 'my_films', but
>>>     with js
>>>     as a jsonb column)
>>>
>>>     manual says: "ERROR: more than one SQL/JSON item"
>>>       in reality: "ERROR: JSON path expression in JSON_QUERY should
>>>     return
>>>     singleton item without wrapper"
>>>              and:   "HINT: use WITH WRAPPER clause to wrap SQL/JSON item
>>>     sequence into array"
>>>
>>>
>>>     Thanks,
>>>
>>>     Erik Rijkers
>>>
>>>     >
>>>     > --
>>>     > Nikita Glukhov
>>>     > Postgres Professional: http://www.postgrespro.com
>>>     <http://www.postgrespro.com>
>>>     > The Russian Postgres Company
>>>
>>>
>>> The patch (func.sgml.20210123.diff) does not apply successfully.
>>>
>>> http://cfbot.cputube.org/patch_32_2901.log
>>> <http://cfbot.cputube.org/patch_32_2901.log>
>>>
>>> ----
>>> === Applying patches on top of PostgreSQL commit ID 0ce4cd04da558178b0186057b721c50a00b7a945 ===
>>> === applying patch ./func.sgml.20210123.diff
>>> patching file doc/src/sgml/func.sgml
>>> Hunk #1 FAILED at 16968.
>>> Hunk #2 FAILED at 17034.
>>> ...
>>> Hunk #19 FAILED at 18743.
>>> 19 out of 19 hunks FAILED -- saving rejects to file doc/src/sgml/func.sgml.rej
>>> ----
>>>
>>> Can we get a rebase? 
>>>
>>> I am marking the patch "Waiting on Author".
>>
>>
>> I've rebased this, and applied some of Erik's changes.
>>
>>
>> I'll set it back to 'Needs Review' if the cfbot is happy.
>
> It's not. There are errors  when building with llvm. I'll investigate.



Specifically, patch 4 (SQL-JSON-query-functions) fails with this when
built with LLVM:


cache gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type
-Wformat-security -fno-strict-aliasing -fwrapv
-fexcess-precision=standard -Wno-format-truncation
-Wno-stringop-truncation -g -O2  -fPIC -D__STDC_LIMIT_MACROS
-D__STDC_FORMAT_MACROS -D__STDC_CONSTANT_MACROS -D_GNU_SOURCE
-I/usr/include  -I../../../../src/include
-I/home/andrew/pgl/pg_head/src/include  -D_GNU_SOURCE   -c -o
llvmjit_expr.o /home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c
In file included from /home/andrew/pgl/pg_head/src/include/postgres.h:46,
                 from
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:16:
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c: In
function ‘llvm_compile_expr’:
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:2348:30:
warning: initialization of ‘struct LLVMOpaqueValue *’ from incompatible
pointer type ‘ExprEvalStep *’ {aka ‘struct ExprEvalStep *’}
[-Wincompatible-pointer-types]
 2348 |         v_state, v_econtext, op);
      |                              ^~
/home/andrew/pgl/pg_head/src/include/c.h:734:34: note: in definition of
macro ‘lengthof’
  734 | #define lengthof(array) (sizeof (array) / sizeof ((array)[0]))
      |                                  ^~~~~
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:2347:5:
note: in expansion of macro ‘build_EvalXFunc’
 2347 |     build_EvalXFunc(b, mod, "ExecEvalJson",
      |     ^~~~~~~~~~~~~~~
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:2348:30:
note: (near initialization for ‘(anonymous)[0]’)
 2348 |         v_state, v_econtext, op);
      |                              ^~
/home/andrew/pgl/pg_head/src/include/c.h:734:34: note: in definition of
macro ‘lengthof’
  734 | #define lengthof(array) (sizeof (array) / sizeof ((array)[0]))
      |                                  ^~~~~
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:2347:5:
note: in expansion of macro ‘build_EvalXFunc’
 2347 |     build_EvalXFunc(b, mod, "ExecEvalJson",
      |     ^~~~~~~~~~~~~~~
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:2348:30:
warning: initialization of ‘struct LLVMOpaqueValue *’ from incompatible
pointer type ‘ExprEvalStep *’ {aka ‘struct ExprEvalStep *’}
[-Wincompatible-pointer-types]
 2348 |         v_state, v_econtext, op);
      |                              ^~
/home/andrew/pgl/pg_head/src/include/c.h:734:52: note: in definition of
macro ‘lengthof’
  734 | #define lengthof(array) (sizeof (array) / sizeof ((array)[0]))
      |                                                    ^~~~~
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:2347:5:
note: in expansion of macro ‘build_EvalXFunc’
 2347 |     build_EvalXFunc(b, mod, "ExecEvalJson",
      |     ^~~~~~~~~~~~~~~
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:2348:30:
note: (near initialization for ‘(anonymous)[0]’)
 2348 |         v_state, v_econtext, op);
      |                              ^~
/home/andrew/pgl/pg_head/src/include/c.h:734:52: note: in definition of
macro ‘lengthof’
  734 | #define lengthof(array) (sizeof (array) / sizeof ((array)[0]))
      |                                                    ^~~~~
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:2347:5:
note: in expansion of macro ‘build_EvalXFunc’
 2347 |     build_EvalXFunc(b, mod, "ExecEvalJson",
      |     ^~~~~~~~~~~~~~~
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:2348:30:
warning: initialization of ‘struct LLVMOpaqueValue *’ from incompatible
pointer type ‘ExprEvalStep *’ {aka ‘struct ExprEvalStep *’}
[-Wincompatible-pointer-types]
 2348 |         v_state, v_econtext, op);
      |                              ^~
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:71:27:
note: in definition of macro ‘build_EvalXFunc’
   71 |         ((LLVMValueRef[]){__VA_ARGS__}))
      |                           ^~~~~~~~~~~
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:2348:30:
note: (near initialization for ‘(anonymous)[0]’)
 2348 |         v_state, v_econtext, op);
      |                              ^~
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:71:27:
note: in definition of macro ‘build_EvalXFunc’
   71 |         ((LLVMValueRef[]){__VA_ARGS__}))
      |                           ^~~~~~~~~~~
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:2348:18:
warning: passing argument 5 of ‘build_EvalXFuncInt’ from incompatible
pointer type [-Wincompatible-pointer-types]
 2348 |         v_state, v_econtext, op);
      |                  ^~~~~~~~~~
      |                  |
      |                  LLVMValueRef {aka struct LLVMOpaqueValue *}
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:69:48:
note: in definition of macro ‘build_EvalXFunc’
   69 |  build_EvalXFuncInt(b, mod, funcname, v_state, op, \
      |                                                ^~
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:63:27:
note: expected ‘ExprEvalStep *’ {aka ‘struct ExprEvalStep *’} but
argument is of type ‘LLVMValueRef’ {aka ‘struct LLVMOpaqueValue *’}
   63 |             ExprEvalStep *op,
      |             ~~~~~~~~~~~~~~^~
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:2349:29:
error: ‘i’ undeclared (first use in this function)
 2349 |     LLVMBuildBr(b, opblocks[i + 1]);
      |                             ^
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:2349:29:
note: each undeclared identifier is reported only once for each function
it appears in
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:248:3:
warning: enumeration value ‘EEOP_JSON_CONSTRUCTOR’ not handled in switch
[-Wswitch]
  248 |   switch (opcode)
      |   ^~~~~~
/home/andrew/pgl/pg_head/src/backend/jit/llvm/llvmjit_expr.c:248:3:
warning: enumeration value ‘EEOP_IS_JSON’ not handled in switch [-Wswitch]
make[2]: *** [<builtin>: llvmjit_expr.o] Error 1
make[2]: Leaving directory
'/home/andrew/pgl/pg_head/bfroot/HEAD/pgsql.build/src/backend/jit/llvm'
make[1]: *** [Makefile:42: all-backend/jit/llvm-recurse] Error 2
make[1]: Leaving directory
'/home/andrew/pgl/pg_head/bfroot/HEAD/pgsql.build/src'


There is also a bug that results in a warning in gram.y, but fixing it
doesn't affect this issue. Nikita, please look into this ASAP.


cheers


andrew


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

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

Specifically, patch 4 (SQL-JSON-query-functions) fails with this when
built with LLVM:


There is also a bug that results in a warning in gram.y, but fixing it
doesn't affect this issue. Nikita, please look into this ASAP.

LLVM issues and gram.y are fixed.

On 4/28/21 5:55 PM, Andrew Dunstan wrote:
>
>
> On Fri, Mar 26, 2021 at 9:14 PM Nikita Glukhov
> <n.gluhov@postgrespro.ru <mailto:n.gluhov@postgrespro.ru>> wrote:
>
>     Attached 54th version of the patches rebased onto current master.
>
>     On 27.03.2021 01:30, Andrew Dunstan wrote:
>>     Specifically, patch 4 (SQL-JSON-query-functions) fails with this when
>>     built with LLVM:
>>
>>
>>     There is also a bug that results in a warning in gram.y, but fixing it
>>     doesn't affect this issue. Nikita, please look into this ASAP.
>
>     LLVM issues and gram.y are fixed.
>
>
>
>
> It's apparently bitrotted again. See
> <http://cfbot.cputube.org/patch_33_2901.log
> <http://cfbot.cputube.org/patch_33_2901.log>>
>
>


This set should remove the bitrot.


cheers


andrew





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

On 5/8/21 2:21 PM, Andrew Dunstan wrote:
> On 4/28/21 5:55 PM, Andrew Dunstan wrote:
>>
>> On Fri, Mar 26, 2021 at 9:14 PM Nikita Glukhov
>> <n.gluhov@postgrespro.ru <mailto:n.gluhov@postgrespro.ru>> wrote:
>>
>>     Attached 54th version of the patches rebased onto current master.
>>
>>     On 27.03.2021 01:30, Andrew Dunstan wrote:
>>>     Specifically, patch 4 (SQL-JSON-query-functions) fails with this when
>>>     built with LLVM:
>>>
>>>
>>>     There is also a bug that results in a warning in gram.y, but fixing it
>>>     doesn't affect this issue. Nikita, please look into this ASAP.
>>     LLVM issues and gram.y are fixed.
>>
>>
>>
>>
>> It's apparently bitrotted again. See
>> <http://cfbot.cputube.org/patch_33_2901.log
>> <http://cfbot.cputube.org/patch_33_2901.log>>
>>
>>
>
> This set should remove the bitrot.
>
>



Rebased for removal of serial schedule


cheers


andrew


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

On 5/18/21 3:22 PM, Andrew Dunstan wrote:
> On 5/8/21 2:21 PM, Andrew Dunstan wrote:
>> On 4/28/21 5:55 PM, Andrew Dunstan wrote:
>>> On Fri, Mar 26, 2021 at 9:14 PM Nikita Glukhov
>>> <n.gluhov@postgrespro.ru <mailto:n.gluhov@postgrespro.ru>> wrote:
>>>
>>>     Attached 54th version of the patches rebased onto current master.
>>>
>>>     On 27.03.2021 01:30, Andrew Dunstan wrote:
>>>>     Specifically, patch 4 (SQL-JSON-query-functions) fails with this when
>>>>     built with LLVM:
>>>>
>>>>
>>>>     There is also a bug that results in a warning in gram.y, but fixing it
>>>>     doesn't affect this issue. Nikita, please look into this ASAP.
>>>     LLVM issues and gram.y are fixed.
>>>
>>>
>>>
>>>
>>> It's apparently bitrotted again. See
>>> <http://cfbot.cputube.org/patch_33_2901.log
>>> <http://cfbot.cputube.org/patch_33_2901.log>>
>>>
>>>
>> This set should remove the bitrot.
>>
>>
>
>
> Rebased for removal of serial schedule
>
>

rebased on master and incorporating fixes from Erik Rijkers


cheers


andrew

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

On 9/2/21 2:50 PM, Andrew Dunstan wrote:
> On 5/18/21 3:22 PM, Andrew Dunstan wrote:
>> On 5/8/21 2:21 PM, Andrew Dunstan wrote:
>>> On 4/28/21 5:55 PM, Andrew Dunstan wrote:
>>>> On Fri, Mar 26, 2021 at 9:14 PM Nikita Glukhov
>>>> <n.gluhov@postgrespro.ru <mailto:n.gluhov@postgrespro.ru>> wrote:
>>>>
>>>>     Attached 54th version of the patches rebased onto current master.
>>>>
>>>>     On 27.03.2021 01:30, Andrew Dunstan wrote:
>>>>>     Specifically, patch 4 (SQL-JSON-query-functions) fails with this when
>>>>>     built with LLVM:
>>>>>
>>>>>
>>>>>     There is also a bug that results in a warning in gram.y, but fixing it
>>>>>     doesn't affect this issue. Nikita, please look into this ASAP.
>>>>     LLVM issues and gram.y are fixed.
>>>>
>>>>
>>>>
>>>>
>>>> It's apparently bitrotted again. See
>>>> <http://cfbot.cputube.org/patch_33_2901.log
>>>> <http://cfbot.cputube.org/patch_33_2901.log>>
>>>>
>>>>
>>> This set should remove the bitrot.
>>>
>>>
>>
>> Rebased for removal of serial schedule
>>
>>
> rebased on master and incorporating fixes from Erik Rijkers
>
>

rebased to remove bitrot from the removal of the  Value node type.


cheers


andrew


-- 

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

On 9/14/21 8:55 AM, Andrew Dunstan wrote:
> On 9/2/21 2:50 PM, Andrew Dunstan wrote:
>> On 5/18/21 3:22 PM, Andrew Dunstan wrote:
>>> On 5/8/21 2:21 PM, Andrew Dunstan wrote:
>>>> On 4/28/21 5:55 PM, Andrew Dunstan wrote:
>>>>> On Fri, Mar 26, 2021 at 9:14 PM Nikita Glukhov
>>>>> <n.gluhov@postgrespro.ru <mailto:n.gluhov@postgrespro.ru>> wrote:
>>>>>
>>>>>     Attached 54th version of the patches rebased onto current master.
>>>>>
>>>>>     On 27.03.2021 01:30, Andrew Dunstan wrote:
>>>>>>     Specifically, patch 4 (SQL-JSON-query-functions) fails with this when
>>>>>>     built with LLVM:
>>>>>>
>>>>>>
>>>>>>     There is also a bug that results in a warning in gram.y, but fixing it
>>>>>>     doesn't affect this issue. Nikita, please look into this ASAP.
>>>>>     LLVM issues and gram.y are fixed.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> It's apparently bitrotted again. See
>>>>> <http://cfbot.cputube.org/patch_33_2901.log
>>>>> <http://cfbot.cputube.org/patch_33_2901.log>>
>>>>>
>>>>>
>>>> This set should remove the bitrot.
>>>>
>>>>
>>> Rebased for removal of serial schedule
>>>
>>>
>> rebased on master and incorporating fixes from Erik Rijkers
>>
>>
> rebased to remove bitrot from the removal of the  Value node type.
>
>

Rebased, and fixed a bug (which I had faithfully replicated above) in
the APP_JUMB code, as reported by Erik Rijkers.


cheers


andrew


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

On 12/1/21 06:13, Himanshu Upadhyaya wrote:
> Hi Andrew,
>
> The latest version (v59) is not applying on head.
> Could you please help to rebase?
>
>


(Please don't top-post on PostgreSQL lists)


The patches apply for me and for the cfbot:
<http://cfbot.cputube.org/patch_35_2901.log>. I'm not sure what's not
working for you. I apply them using "patch -p 1 < $patchfile"


cheers


andrew


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

On 09.12.21 15:04, Himanshu Upadhyaya wrote:
> 1)
> Why we don't support KEY(however is optional as per SQL standard) keyword?
> SELECT JSON_OBJECT(KEY 'a' VALUE '123');
> ERROR:  type "key" does not exist
> LINE 1: SELECT JSON_OBJECT(KEY 'a' VALUE '123');
> 
> ORACLE is supporting the above syntax.
> 
> I can see TODO as below
> +json_name_and_value:
> +/* TODO This is not supported due to conflicts
> +                       KEY c_expr VALUE_P json_value_expr %prec POSTFIXOP
> +                               { $$ = makeJsonKeyValue($2, $4); }
> +                       |
> +*/
> 
> but still not very clear what kind of conflict we are mentioning here, 
> also any plan of finding a solution to that conflict?

The conflict is this:

Consider in subclause 6.33, “<JSON value constructor>”:

<JSON name and value> ::= [ KEY ] <JSON name> VALUE <JSON input expression>
| ...

Because KEY is a <non-reserved word>, this creates an ambiguity. For 
example:

key(x) VALUE foo

could be

KEY x VALUE foo

with KEY being the key word and “x” (a <column reference>) as “<JSON 
name>”, or

KEY key(x) VALUE foo

with “key(x)” (a <routine invocation>) as “<JSON name>”.

In existing implementations, KEY is resolved as a keyword.  So if you 
can figure out a way to implement that, go ahead, but I imagine it might 
be tricky.

On 12/9/21 09:04, Himanshu Upadhyaya wrote:
>
>
>
> 4)
> Are we intentionally allowing numeric keys in JSON_OBJECT but somehow
> these are not allowed in ORACLE?
> ‘postgres[151876]=#’select JSON_OBJECT( 3+1:2, 2+2:1);
>     json_object
> --------------------
>  {"4" : 2, "4" : 1}
> (1 row)
>
> In ORACLE we are getting error("ORA-00932: inconsistent datatypes:
> expected CHAR got NUMBER") which seems to be more reasonable.
> "ORA-00932: inconsistent datatypes: expected CHAR got NUMBER"
>
> Postgres is also dis-allowing below then why allow numeric keys in
> JSON_OBJECT?
> ‘postgres[151876]=#’select '{
>   "track": {
>     "segments": [
>       {
>         "location":   [ 47.763, 13.4034 ],
>         "start time": "2018-10-14 10:05:14",
>         "HR": 73
>       },
>       {
>         "location":   [ 47.706, 13.2635 ],
>         "start time": "2018-10-14 10:39:21",
>         3: 135
>       }
>     ]
>   }
> }'::jsonb;
> ERROR:  22P02: invalid input syntax for type json
> LINE 1: select '{
>                ^
> DETAIL:  Expected string, but found "3".
> CONTEXT:  JSON data, line 12:         3...
> LOCATION:  json_ereport_error, jsonfuncs.c:621
>
> Also, JSON_OBJECTAGG is failing if we have any numeric key, however,
> the message is not very appropriate.
> SELECT JSON_OBJECTAGG(k: v ABSENT ON NULL) AS apt
> FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', NULL),
> (5,5)) kv(k, v);
> ERROR:  22P02: invalid input syntax for type integer: "no"
> LINE 2: FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', ...
>                       ^
> LOCATION:  pg_strtoint32, numutils.c:320
>
>
>

The literal above is simply not legal json, so the json parser is going
to reject it outright. However it is quite reasonable for JSON
constructors to convert non-string key values to strings. Otherwise we'd
be rejecting not just numbers but for example dates as key values. c.f.
json_build_object(), the documentation for which says "Key arguments are
coerced to text."


cheers


andrew

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

On 12/9/21 09:04, Himanshu Upadhyaya wrote:
>
>
>
> 2)
> I am not sure if below is required as per SQL standard, ORACLE is
> allowing to construct JSON_OBJECT bases on the records in the table as
> below, but postgres parser is not allowing:
> create table test (id varchar(10), value int);
> insert into test values ('a',1);
> insert into test values ('b',2);
> insert into test values ('c',3);
> select json_object(*) from test; --postgres does not support
> postgres=# select json_object(*) from test;
> ERROR:  syntax error at or near "*"
> LINE 1: select json_object(*) from test;
>
>

You can spell that a bit differently today, e.g.

    select to_json(r) from test r;

I don't know either if it's in the spec, but building in support for *
in this context seems likely to be fairly complex and have very little
added utility.


cheers


andrew

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

On Thu, Dec 16, 2021 at 3:06 AM Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 12/9/21 09:04, Himanshu Upadhyaya wrote:
> >
> >
> >
> > 4)
> > Are we intentionally allowing numeric keys in JSON_OBJECT but somehow
> > these are not allowed in ORACLE?
> > ‘postgres[151876]=#’select JSON_OBJECT( 3+1:2, 2+2:1);
> >     json_object
> > --------------------
> >  {"4" : 2, "4" : 1}
> > (1 row)
> >
> > In ORACLE we are getting error("ORA-00932: inconsistent datatypes:
> > expected CHAR got NUMBER") which seems to be more reasonable.
> > "ORA-00932: inconsistent datatypes: expected CHAR got NUMBER"
> >
> > Postgres is also dis-allowing below then why allow numeric keys in
> > JSON_OBJECT?
> > ‘postgres[151876]=#’select '{
> >   "track": {
> >     "segments": [
> >       {
> >         "location":   [ 47.763, 13.4034 ],
> >         "start time": "2018-10-14 10:05:14",
> >         "HR": 73
> >       },
> >       {
> >         "location":   [ 47.706, 13.2635 ],
> >         "start time": "2018-10-14 10:39:21",
> >         3: 135
> >       }
> >     ]
> >   }
> > }'::jsonb;
> > ERROR:  22P02: invalid input syntax for type json
> > LINE 1: select '{
> >                ^
> > DETAIL:  Expected string, but found "3".
> > CONTEXT:  JSON data, line 12:         3...
> > LOCATION:  json_ereport_error, jsonfuncs.c:621
> >
> > Also, JSON_OBJECTAGG is failing if we have any numeric key, however,
> > the message is not very appropriate.
> > SELECT JSON_OBJECTAGG(k: v ABSENT ON NULL) AS apt
> > FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', NULL),
> > (5,5)) kv(k, v);
> > ERROR:  22P02: invalid input syntax for type integer: "no"
> > LINE 2: FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', ...
> >                       ^
> > LOCATION:  pg_strtoint32, numutils.c:320
> >
> >
> >
>
> The literal above is simply not legal json, so the json parser is going
> to reject it outright. However it is quite reasonable for JSON
> constructors to convert non-string key values to strings. Otherwise we'd
> be rejecting not just numbers but for example dates as key values. c.f.
> json_build_object(), the documentation for which says "Key arguments are
> coerced to text."
>
Yes Agree on this, but just thinking if we can differentiate dates and
numeric keys to have consistent behaviour and simply reject if we have
numeric keys(to match it with the behaviour of JSON parser) because
JSON with numeric keys is actually not a valid JSON.

SELECT JSON_OBJECTAGG(k: v ABSENT ON NULL) AS apt
FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', NULL),
(5,5)) kv(k, v);
ERROR:  22P02: invalid input syntax for type integer: "no"
LINE 2: FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', ...
                      ^
LOCATION:  pg_strtoint32, numutils.c:320

Above call to JSON_OBJECTAGG is failing because we have the numeric
key, is not that it also needs to follow the same context  of
converting key argument to text? or both(JSON_OBJECTAGG  and
JSON_OBJECT) should not allow numeric keys in the JSON object and
allow date (if that is the only use case)?

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

On 1/4/22 04:18, Himanshu Upadhyaya wrote:
> On Thu, Dec 16, 2021 at 3:06 AM Andrew Dunstan <andrew@dunslane.net> wrote:
>>
>>
>> SELECT JSON_OBJECTAGG(k: v ABSENT ON NULL) AS apt
>> FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', NULL),
>> (5,5)) kv(k, v);
>> ERROR:  22P02: invalid input syntax for type integer: "no"
>> LINE 2: FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2), ('foo', ...
>>                       ^
>> LOCATION:  pg_strtoint32, numutils.c:320
>>
>> Above call to JSON_OBJECTAGG is failing because we have the numeric
>> key, is not that it also needs to follow the same context  of
>> converting key argument to text? or both(JSON_OBJECTAGG  and
>> JSON_OBJECT) should not allow numeric keys in the JSON object and
>> allow date (if that is the only use case)?
>>


this error has nothing at all to do with the json code. You simply have
an invalid VALUES expression:


postgres=# select * FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2),
('foo', NULL), (5,5)) kv(k, v);
ERROR:  invalid input syntax for type integer: "no"
LINE 1: select * FROM (VALUES ('no', 5), ('area', 50), ('rooms', 2),...

cheers


andrew

-- 

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

On 9/16/21 10:52, Andrew Dunstan wrote:
> On 9/14/21 8:55 AM, Andrew Dunstan wrote:
>> On 9/2/21 2:50 PM, Andrew Dunstan wrote:
>>> On 5/18/21 3:22 PM, Andrew Dunstan wrote:
>>>> On 5/8/21 2:21 PM, Andrew Dunstan wrote:
>>>>> On 4/28/21 5:55 PM, Andrew Dunstan wrote:
>>>>>> On Fri, Mar 26, 2021 at 9:14 PM Nikita Glukhov
>>>>>> <n.gluhov@postgrespro.ru <mailto:n.gluhov@postgrespro.ru>> wrote:
>>>>>>
>>>>>>     Attached 54th version of the patches rebased onto current master.
>>>>>>
>>>>>>     On 27.03.2021 01:30, Andrew Dunstan wrote:
>>>>>>>     Specifically, patch 4 (SQL-JSON-query-functions) fails with this when
>>>>>>>     built with LLVM:
>>>>>>>
>>>>>>>
>>>>>>>     There is also a bug that results in a warning in gram.y, but fixing it
>>>>>>>     doesn't affect this issue. Nikita, please look into this ASAP.
>>>>>>     LLVM issues and gram.y are fixed.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> It's apparently bitrotted again. See
>>>>>> <http://cfbot.cputube.org/patch_33_2901.log
>>>>>> <http://cfbot.cputube.org/patch_33_2901.log>>
>>>>>>
>>>>>>
>>>>> This set should remove the bitrot.
>>>>>
>>>>>
>>>> Rebased for removal of serial schedule
>>>>
>>>>
>>> rebased on master and incorporating fixes from Erik Rijkers
>>>
>>>
>> rebased to remove bitrot from the removal of the  Value node type.
>>
>>
> Rebased, and fixed a bug (which I had faithfully replicated above) in
> the APP_JUMB code, as reported by Erik Rijkers.
>
>

rebased once more


cheers


andrew


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

On Thu, Dec 9, 2021 at 7:34 PM Himanshu Upadhyaya
<upadhyaya.himanshu@gmail.com> wrote:
> 3)
> Is not that result of the two below queries should match because both are trying to retrieve the information from the
JSONobject.
 
>
> postgres=# SELECT JSON_OBJECT('track' VALUE '{
>     "segments": [
>       {
>         "location":   [ 47.763, 13.4034 ],
>         "start time": "2018-10-14 10:05:14",
>         "HR": 73
>       },
>       {
>         "location":   [ 47.706, 13.2635 ],
>         "start time": "2018-10-14 101:39:21",
>         "HR": 135
>       }
>     ]
>   }
> }')->'track'->'segments';
>  ?column?
> ----------
>
> (1 row)
>
> postgres=# select '{
>   "track": {
>     "segments": [
>       {
>         "location":   [ 47.763, 13.4034 ],
>         "start time": "2018-10-14 10:05:14",
>         "HR": 73
>       },
>       {
>         "location":   [ 47.706, 13.2635 ],
>         "start time": "2018-10-14 10:39:21",
>         "HR": 135
>       }
>     ]
>   }
> }'::jsonb->'track'->'segments';
>                                                                              ?column?
>
-------------------------------------------------------------------------------------------------------------------------------------------------------------------
>  [{"HR": 73, "location": [47.763, 13.4034], "start time": "2018-10-14 10:05:14"}, {"HR": 135, "location": [47.706,
13.2635],"start time": "2018-10-14 10:39:21"}]
 
> (1 row)
>
just wanted to check your opinion on the above, is this an expected behaviour?

> Few comments For 0002-SQL-JSON-constructors-v59.patch:
Also, any thoughts on this?

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

On 1/5/22 00:51, Himanshu Upadhyaya wrote:
> On Thu, Dec 9, 2021 at 7:34 PM Himanshu Upadhyaya
> <upadhyaya.himanshu@gmail.com> wrote:
>> 3)
>> Is not that result of the two below queries should match because both are trying to retrieve the information from
theJSON object.
 
>>
>> postgres=# SELECT JSON_OBJECT('track' VALUE '{
>>     "segments": [
>>       {
>>         "location":   [ 47.763, 13.4034 ],
>>         "start time": "2018-10-14 10:05:14",
>>         "HR": 73
>>       },
>>       {
>>         "location":   [ 47.706, 13.2635 ],
>>         "start time": "2018-10-14 101:39:21",
>>         "HR": 135
>>       }
>>     ]
>>   }
>> }')->'track'->'segments';
>>  ?column?
>> ----------
>>
>> (1 row)
>>
>> postgres=# select '{
>>   "track": {
>>     "segments": [
>>       {
>>         "location":   [ 47.763, 13.4034 ],
>>         "start time": "2018-10-14 10:05:14",
>>         "HR": 73
>>       },
>>       {
>>         "location":   [ 47.706, 13.2635 ],
>>         "start time": "2018-10-14 10:39:21",
>>         "HR": 135
>>       }
>>     ]
>>   }
>> }'::jsonb->'track'->'segments';
>>                                                                              ?column?
>>
-------------------------------------------------------------------------------------------------------------------------------------------------------------------
>>  [{"HR": 73, "location": [47.763, 13.4034], "start time": "2018-10-14 10:05:14"}, {"HR": 135, "location": [47.706,
13.2635],"start time": "2018-10-14 10:39:21"}]
 
>> (1 row)
>>
> just wanted to check your opinion on the above, is this an expected behaviour?


Your VALUE clause is actually not legal JSON - it has one too many
braces at the end. The reason postgres didn't complain about it is that
JSON_OBJECT is treating it as a string. If you correct the JSON and cast
it as jsonb you get the desired result:

andrew=# SELECT JSON_OBJECT('track' VALUE '{
    "segments": [
      {
        "location":   [ 47.763, 13.4034 ],
        "start time": "2018-10-14 10:05:14",
        "HR": 73
      },
      {
        "location":   [ 47.706, 13.2635 ],
        "start time": "2018-10-14 101:39:21",
        "HR": 135
      }
    ]
  }'::jsonb)->'track'->'segments';
                                                                              ?column?
                                           
 

--------------------------------------------------------------------------------------------------------------------------------------------------------------------
 [{"HR": 73, "location": [47.763, 13.4034], "start time": "2018-10-14 10:05:14"}, {"HR": 135, "location": [47.706,
13.2635],"start time": "2018-10-14 101:39:21"}]
 
(1 row)


>> Few comments For 0002-SQL-JSON-constructors-v59.patch:
> Also, any thoughts on this?



I will look at that separately.


cheers


andrew


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

On Tue, Jan 4, 2022 at 7:32 PM Andrew Dunstan <andrew@dunslane.net> wrote:

I have one general question on the below scenario.
CREATE TABLE T (Id INTEGER PRIMARY KEY,Jcol CHARACTER VARYING ( 5000
)CHECK ( Jcol IS JSON ) );
insert into T values (1,323);
 ORACLE is giving an error(check constraint...violated ORA-06512) for
the above insert but Postgres is allowing it, however is not related
to this patch but just thinking if this is expected.

‘postgres[22198]=#’SELECT * FROM T WHERE Jcol IS JSON;
 id | jcol
----+------
  1 | 323
How come number 323 is the valid json?

Few comments/doubts on 0003-IS-JSON-predicate-v59.patch and
0004-SQL-JSON-query-functions-v59.patch patch:
1) I am not able to find a case where "IS JSON" and "IS JSON VALUE"
gives a different result, is they intended to give the same result(and
two are replaceably used) when applied on any input.

2) Not sure why we return true for the below query?
+-- extension: boolean expressions
+SELECT JSON_EXISTS(jsonb '1', '$ > 2');
+ json_exists
+-------------
+ t
+(1 row)

3)
+-- Strict mode with ERROR on ERROR clause
+SELECT JSON_EXISTS(jsonb '{"a": [1,2,3]}', 'strict $.a[5]' ERROR ON ERROR);
+ERROR: Invalid SQL/JSON subscript

The above example in documentation is not actually matching when I am
trying to run with the patch as below.
‘postgres[28411]=#’SELECT JSON_EXISTS(jsonb '{"a": [1,2,3]}', 'strict
$.a[5]' ERROR ON ERROR);
ERROR:  22033: jsonpath array subscript is out of bounds
LOCATION:  executeItemOptUnwrapTarget, jsonpath_exec.c:769

+SELECT JSON_VALUE('"123.45"', '$' RETURNING float);
+ json_value
+------------
+     123.45
+(1 row)

Above is also not matching:
‘postgres[28411]=#’SELECT JSON_VALUE('"123.45"', '$' RETURNING float);
ERROR:  0A000: JSON_VALUE() is not yet implemented for json type
LINE 1: SELECT JSON_VALUE('"123.45"', '$' RETURNING float);

There is more such example that does not actually produce the same
result when we try to run after applying this patch, seems like we
just need to update the documentation with regards to our new patch.
+SELECT JSON_VALUE(jsonb '[1,2]', 'strict $[*]' ERROR ON ERROR);
+ERROR: more than one SQL/JSON item

‘postgres[28411]=#’SELECT JSON_VALUE(jsonb '[1,2]', 'strict $[*]'
ERROR ON ERROR);
ERROR:  22034: JSON path expression in JSON_VALUE should return
singleton scalar item

4)
index f46786231e..c1951c1caf 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -28,6 +28,7 @@
 #include "catalog/pg_type.h"
 #include "executor/executor.h"
 #include "executor/functions.h"
+#include "executor/execExpr.h"
 #include "funcapi.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"

can we adjust the include file in the alphabetic order please?

5)
+SELECT
+       JSON_QUERY(js, '$'),
+       JSON_QUERY(js, '$' WITHOUT WRAPPER),
+       JSON_QUERY(js, '$' WITH CONDITIONAL WRAPPER),
+       JSON_QUERY(js, '$' WITH UNCONDITIONAL ARRAY WRAPPER),
+       JSON_QUERY(js, '$' WITH ARRAY WRAPPER)
+FROM
+       (VALUES
+               (jsonb 'null'),
+               ('12.3'),
+               ('true'),
+               ('"aaa"'),
+               ('[1, null, "2"]'),
+               ('{"a": 1, "b": [2]}')
+       ) foo(js);
+     json_query     |     json_query     |     json_query     |
json_query      |      json_query
+--------------------+--------------------+--------------------+----------------------+----------------------
+ null               | null               | [null]             |
[null]               | [null]
+ 12.3               | 12.3               | [12.3]             |
[12.3]               | [12.3]
+ true               | true               | [true]             |
[true]               | [true]
+ "aaa"              | "aaa"              | ["aaa"]            |
["aaa"]              | ["aaa"]
+ [1, null, "2"]     | [1, null, "2"]     | [1, null, "2"]     | [[1,
null, "2"]]     | [[1, null, "2"]]
+ {"a": 1, "b": [2]} | {"a": 1, "b": [2]} | {"a": 1, "b": [2]} |
[{"a": 1, "b": [2]}] | [{"a": 1, "b": [2]}]
+(6 rows)

Just a suggestion if we can have column aliases for better
understanding like we are doing for other test cases in the same
patch?

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

On 1/6/22 06:24, Himanshu Upadhyaya wrote:
> I have one general question on the below scenario.
> CREATE TABLE T (Id INTEGER PRIMARY KEY,Jcol CHARACTER VARYING ( 5000
> )CHECK ( Jcol IS JSON ) );
> insert into T values (1,323);
>  ORACLE is giving an error(check constraint...violated ORA-06512) for
> the above insert but Postgres is allowing it, however is not related
> to this patch but just thinking if this is expected.
>
> ‘postgres[22198]=#’SELECT * FROM T WHERE Jcol IS JSON;
>  id | jcol
> ----+------
>   1 | 323
> How come number 323 is the valid json?


If you look at the JSON grammar at <https://www.json.org/json-en.html>
or
<https://www.ecma-international.org/wp-content/uploads/ECMA-404_2nd_edition_december_2017.pdf>
it's clear that a bare number is valid json. Our parser implements that
grammar pretty faithfully, in fact rather more faithfully than many
implementations (e.g. we allow huge number strings). So as far as I'm
concerned, we are right and Oracle is wrong. It would hardly be the
first time such a thing has happened.

Oracle is not the definer of the JSON standard. ECMA is.


cheers


andrew


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

On 12/9/21 09:04, Himanshu Upadhyaya wrote:
>
>
>
> Few comments For 0002-SQL-JSON-constructors-v59.patch:
> 1)
> +       if (IsA(node, JsonConstructorExpr))
> +       {
> +               JsonConstructorExpr *ctor = (JsonConstructorExpr *) node;
> +               ListCell   *lc;
> +               bool            is_jsonb =
> +                       ctor->returning->format->format ==
> JS_FORMAT_JSONB;
> +
> +               /* Check argument_type => json[b] conversions */
> +               foreach(lc, ctor->args)
> +               {
> +                       Oid                     typid =
> exprType(lfirst(lc));
> +
> +                       if (is_jsonb ?
> +                               !to_jsonb_is_immutable(typid) :
> +                               !to_json_is_immutable(typid))
> +                               return true;
> +               }
> +
> +               /* Check all subnodes */
> +       }
> can have ctor as const pointer?


I guess we could, although I'm not sure it's an important advance.


>
> 2)
> +typedef struct JsonFormat
> +{
> +       NodeTag         type;
> +       JsonFormatType format;          /* format type */
> +       JsonEncoding encoding;          /* JSON encoding */
> +       int                     location;               /* token
> location, or -1 if unknown */
> +} JsonFormat;
>
> I think it will be good if we can have a JsonformatType(defined in
> patch 0001-Common-SQL-JSON-clauses-v59.patch) member named as
> format_type or formatType instead of format?
> There are places in the patch where we access it as "if
> (format->format == JS_FORMAT_DEFAULT)". "format->format" looks little
> difficult to understand.
> "format->format_type == JS_FORMAT_DEFAULT" will be easy to follow.


That's a fair criticism.



>
> 3)
> +               if (have_jsonb)
> +               {
> +                       returning->typid = JSONBOID;
> +                       returning->format->format = JS_FORMAT_JSONB;
> +               }
> +               else if (have_json)
> +               {
> +                       returning->typid = JSONOID;
> +                       returning->format->format = JS_FORMAT_JSON;
> +               }
> +               else
> +               {
> +                       /* XXX TEXT is default by the standard, but we
> return JSON */
> +                       returning->typid = JSONOID;
> +                       returning->format->format = JS_FORMAT_JSON;
> +               }
>
> why we need a separate "else if (have_json)" statement in the below
> code, "else" is also doing the same thing?



I imagine it's more or less a placeholder in case we want to do
something else in the default case. But I agree it's confusing.


>
> 4)
> -test: json jsonb json_encoding jsonpath jsonpath_encoding jsonb_jsonpath
> +test: json jsonb json_encoding jsonpath jsonpath_encoding
> jsonb_jsonpath sqljson
>
> can we rename sqljson sql test file to json_constructor?


Not really - the later patches in the series add to it, so it ends up
being more than just constructor tests.


Thanks for reviewing,


cheers


andrew

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

Hi,

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

On 1/18/22 01:33, Julien Rouhaud wrote:
> Hi,
>
> The last version conflicts with recent c4cc2850f4d1 (Rename value node fields).
> Can you send a rebased version?


Attached. I didn't make the minor changes discussed with Himanshu
upthread. They aren't terribly urgent, but I haven't forgotten them.


cheers


andrew

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

On 1/12/22 15:49, Andrew Dunstan wrote:
> On 12/9/21 09:04, Himanshu Upadhyaya wrote:
>>
>>
>> Few comments For 0002-SQL-JSON-constructors-v59.patch:
>> 1)
>> +       if (IsA(node, JsonConstructorExpr))
>> +       {
>> +               JsonConstructorExpr *ctor = (JsonConstructorExpr *) node;
>> +               ListCell   *lc;
>> +               bool            is_jsonb =
>> +                       ctor->returning->format->format ==
>> JS_FORMAT_JSONB;
>> +
>> +               /* Check argument_type => json[b] conversions */
>> +               foreach(lc, ctor->args)
>> +               {
>> +                       Oid                     typid =
>> exprType(lfirst(lc));
>> +
>> +                       if (is_jsonb ?
>> +                               !to_jsonb_is_immutable(typid) :
>> +                               !to_json_is_immutable(typid))
>> +                               return true;
>> +               }
>> +
>> +               /* Check all subnodes */
>> +       }
>> can have ctor as const pointer?
>
> I guess we could, although I'm not sure it's an important advance.
>
>
>> 2)
>> +typedef struct JsonFormat
>> +{
>> +       NodeTag         type;
>> +       JsonFormatType format;          /* format type */
>> +       JsonEncoding encoding;          /* JSON encoding */
>> +       int                     location;               /* token
>> location, or -1 if unknown */
>> +} JsonFormat;
>>
>> I think it will be good if we can have a JsonformatType(defined in
>> patch 0001-Common-SQL-JSON-clauses-v59.patch) member named as
>> format_type or formatType instead of format?
>> There are places in the patch where we access it as "if
>> (format->format == JS_FORMAT_DEFAULT)". "format->format" looks little
>> difficult to understand.
>> "format->format_type == JS_FORMAT_DEFAULT" will be easy to follow.
>
> That's a fair criticism.
>
>
>
>> 3)
>> +               if (have_jsonb)
>> +               {
>> +                       returning->typid = JSONBOID;
>> +                       returning->format->format = JS_FORMAT_JSONB;
>> +               }
>> +               else if (have_json)
>> +               {
>> +                       returning->typid = JSONOID;
>> +                       returning->format->format = JS_FORMAT_JSON;
>> +               }
>> +               else
>> +               {
>> +                       /* XXX TEXT is default by the standard, but we
>> return JSON */
>> +                       returning->typid = JSONOID;
>> +                       returning->format->format = JS_FORMAT_JSON;
>> +               }
>>
>> why we need a separate "else if (have_json)" statement in the below
>> code, "else" is also doing the same thing?
>
>
> I imagine it's more or less a placeholder in case we want to do
> something else in the default case. But I agree it's confusing.
>
>
>> 4)
>> -test: json jsonb json_encoding jsonpath jsonpath_encoding jsonb_jsonpath
>> +test: json jsonb json_encoding jsonpath jsonpath_encoding
>> jsonb_jsonpath sqljson
>>
>> can we rename sqljson sql test file to json_constructor?
>
> Not really - the later patches in the series add to it, so it ends up
> being more than just constructor tests.
>
>
> Thanks for reviewing,
>
>


Here's a patch set with all of these except the last fixed.

There are a couple of things that bother me:

1. This involves a sizeable addition to the grammar, with something over
20 new keywords, including "string" as a  TYPE_FUNC_NAME_KEYWORD. I
guess we can't control what the SQL Standards Committee does, so if we
want to implement this we just need to suck it up. But it's annoying
that they are so profligate with grammar symbols.

2. The new GUC "sql_json" is a bit of a worry. I understand what it's
trying to do, but I'm trying to convince myself it's not going to be a
fruitful source of error reports, especially if people switch it in the
middle of a session. Maybe it should be an initdb option instead of a GUC?


cheers


andrew

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

On 2/1/22 14:11,I wrote:
>
> 2. The new GUC "sql_json" is a bit of a worry. I understand what it's
> trying to do, but I'm trying to convince myself it's not going to be a
> fruitful source of error reports, especially if people switch it in the
> middle of a session. Maybe it should be an initdb option instead of a GUC?
>
>


So far my efforts have not borne fruit. Here's why:


andrew=# set sql_json = jsonb;
SET
andrew=# create table abc (x text, y json);
CREATE TABLE
andrew=# \d abc
               Table "public.abc"
 Column | Type | Collation | Nullable | Default
--------+------+-----------+----------+---------
 x      | text |           |          |
 y      | json |           |          |

andrew=# insert into abc values ('a','{"q":1}');
INSERT 0 1
andrew=# select json_each(y) from abc;
ERROR:  function json_each(json) does not exist
LINE 1: select json_each(y) from abc;
               ^
HINT:  No function matches the given name and argument types. You might
need to add explicit type casts.
andrew=# select jsonb_each(y) from abc;
 jsonb_each
------------
 (q,1)
(1 row)


The description tells them the column is json, but the json_* functions
don't work on the column and you need to use the jsonb functions. That
seems to me a recipe for major confusion. It might be better if we set
it at initdb time so it couldn't be changed, but even so it could be
horribly confusing.

This is certainly severable from the rest of these patches. I'm not sure
how severable it is from the SQL/JSON Table patches.


cheers


andrew

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

On 3/1/22 16:41, Andrew Dunstan wrote:
> On 2/1/22 14:11,I wrote:
>> 2. The new GUC "sql_json" is a bit of a worry. I understand what it's
>> trying to do, but I'm trying to convince myself it's not going to be a
>> fruitful source of error reports, especially if people switch it in the
>> middle of a session. Maybe it should be an initdb option instead of a GUC?
>>
>>
>
> So far my efforts have not borne fruit. Here's why:
>
>
> andrew=# set sql_json = jsonb;
> SET
> andrew=# create table abc (x text, y json);
> CREATE TABLE
> andrew=# \d abc
>                Table "public.abc"
>  Column | Type | Collation | Nullable | Default
> --------+------+-----------+----------+---------
>  x      | text |           |          |
>  y      | json |           |          |
>
> andrew=# insert into abc values ('a','{"q":1}');
> INSERT 0 1
> andrew=# select json_each(y) from abc;
> ERROR:  function json_each(json) does not exist
> LINE 1: select json_each(y) from abc;
>                ^
> HINT:  No function matches the given name and argument types. You might
> need to add explicit type casts.
> andrew=# select jsonb_each(y) from abc;
>  jsonb_each
> ------------
>  (q,1)
> (1 row)
>
>
> The description tells them the column is json, but the json_* functions
> don't work on the column and you need to use the jsonb functions. That
> seems to me a recipe for major confusion. It might be better if we set
> it at initdb time so it couldn't be changed, but even so it could be
> horribly confusing.
>
> This is certainly severable from the rest of these patches. I'm not sure
> how severable it is from the SQL/JSON Table patches.
>
>

I have confirmed that this is not required at all for the JSON_TABLE
patch set.


I'll submit new patch sets omitting it shortly. The GUC patch can be
considered separately, probably as release 16 material, but I think as
is it's at best quite incomplete.


cheers


andrew

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

On 3/2/22 10:19, Andrew Dunstan wrote:
> On 3/1/22 16:41, Andrew Dunstan wrote:
>> On 2/1/22 14:11,I wrote:
>>> 2. The new GUC "sql_json" is a bit of a worry. I understand what it's
>>> trying to do, but I'm trying to convince myself it's not going to be a
>>> fruitful source of error reports, especially if people switch it in the
>>> middle of a session. Maybe it should be an initdb option instead of a GUC?
>>>
>>>
>> So far my efforts have not borne fruit. Here's why:
>>
>>
>> andrew=# set sql_json = jsonb;
>> SET
>> andrew=# create table abc (x text, y json);
>> CREATE TABLE
>> andrew=# \d abc
>>                Table "public.abc"
>>  Column | Type | Collation | Nullable | Default
>> --------+------+-----------+----------+---------
>>  x      | text |           |          |
>>  y      | json |           |          |
>>
>> andrew=# insert into abc values ('a','{"q":1}');
>> INSERT 0 1
>> andrew=# select json_each(y) from abc;
>> ERROR:  function json_each(json) does not exist
>> LINE 1: select json_each(y) from abc;
>>                ^
>> HINT:  No function matches the given name and argument types. You might
>> need to add explicit type casts.
>> andrew=# select jsonb_each(y) from abc;
>>  jsonb_each
>> ------------
>>  (q,1)
>> (1 row)
>>
>>
>> The description tells them the column is json, but the json_* functions
>> don't work on the column and you need to use the jsonb functions. That
>> seems to me a recipe for major confusion. It might be better if we set
>> it at initdb time so it couldn't be changed, but even so it could be
>> horribly confusing.
>>
>> This is certainly severable from the rest of these patches. I'm not sure
>> how severable it is from the SQL/JSON Table patches.
>>
>>
> I have confirmed that this is not required at all for the JSON_TABLE
> patch set.
>
>
> I'll submit new patch sets omitting it shortly. The GUC patch can be
> considered separately, probably as release 16 material, but I think as
> is it's at best quite incomplete.


here's a new set of patches, omitting the GUC patch and with the
beginnings of some message cleanup - there's more work to do there.


cheers


andrew


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

On 3/4/22 11:28, Andrew Dunstan wrote:
> On 3/2/22 10:19, Andrew Dunstan wrote:
>> On 3/1/22 16:41, Andrew Dunstan wrote:
>>> On 2/1/22 14:11,I wrote:
>>>> 2. The new GUC "sql_json" is a bit of a worry. I understand what it's
>>>> trying to do, but I'm trying to convince myself it's not going to be a
>>>> fruitful source of error reports, especially if people switch it in the
>>>> middle of a session. Maybe it should be an initdb option instead of a GUC?
>>>>
>>>>
>>> So far my efforts have not borne fruit. Here's why:
>>>
>>>
>>> andrew=# set sql_json = jsonb;
>>> SET
>>> andrew=# create table abc (x text, y json);
>>> CREATE TABLE
>>> andrew=# \d abc
>>>                Table "public.abc"
>>>  Column | Type | Collation | Nullable | Default
>>> --------+------+-----------+----------+---------
>>>  x      | text |           |          |
>>>  y      | json |           |          |
>>>
>>> andrew=# insert into abc values ('a','{"q":1}');
>>> INSERT 0 1
>>> andrew=# select json_each(y) from abc;
>>> ERROR:  function json_each(json) does not exist
>>> LINE 1: select json_each(y) from abc;
>>>                ^
>>> HINT:  No function matches the given name and argument types. You might
>>> need to add explicit type casts.
>>> andrew=# select jsonb_each(y) from abc;
>>>  jsonb_each
>>> ------------
>>>  (q,1)
>>> (1 row)
>>>
>>>
>>> The description tells them the column is json, but the json_* functions
>>> don't work on the column and you need to use the jsonb functions. That
>>> seems to me a recipe for major confusion. It might be better if we set
>>> it at initdb time so it couldn't be changed, but even so it could be
>>> horribly confusing.
>>>
>>> This is certainly severable from the rest of these patches. I'm not sure
>>> how severable it is from the SQL/JSON Table patches.
>>>
>>>
>> I have confirmed that this is not required at all for the JSON_TABLE
>> patch set.
>>
>>
>> I'll submit new patch sets omitting it shortly. The GUC patch can be
>> considered separately, probably as release 16 material, but I think as
>> is it's at best quite incomplete.
>
> here's a new set of patches, omitting the GUC patch and with the
> beginnings of some message cleanup - there's more work to do there.
>
>


This patchset restores the RETURNING clause for JSON() and JSON_SCALAR()
but without the GUC


cheers


andrew

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

On 3/5/22 09:39, Andrew Dunstan wrote:
>
> here's a new set of patches, omitting the GUC patch and with the
> beginnings of some message cleanup - there's more work to do there.
>
>
>
> This patchset restores the RETURNING clause for JSON() and JSON_SCALAR()
> but without the GUC
>
>


I have committed the first of these. That will break the cfbot for this
and the json_table patches. The remainder should be committed in the
following days.


cheers


andew


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

Andrew Dunstan <andrew@dunslane.net> writes:
> I have committed the first of these. That will break the cfbot for this
> and the json_table patches. The remainder should be committed in the
> following days.

That patch is 0-for-three on my buildfarm animals.

            regards, tom lane

I wrote:
> That patch is 0-for-three on my buildfarm animals.

... the reason being that they use -Werror, and this patch spews
a bunch of warnings.  This is not acceptable, especially not in
the middle of a CF when other people are trying to get work done.
Please revert.

            regards, tom lane

Hi,

On 2022-03-22 18:31:39 -0400, Tom Lane wrote:
> I wrote:
> > That patch is 0-for-three on my buildfarm animals.
> 
> ... the reason being that they use -Werror, and this patch spews
> a bunch of warnings.  This is not acceptable, especially not in
> the middle of a CF when other people are trying to get work done.
> Please revert.

There's also https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jabiru&dt=2022-03-22%2022%3A25%3A26
that started failing with
../../preproc/ecpg --regression -I./../../include -I. -o test1.c test1.pgc
test1.pgc:12: ERROR: syntax error at or near "int"
with this commit.

And the bison warnings I complained about on -committers:
https://www.postgresql.org/message-id/20220322223319.so4ajcki7wwaujin%40alap3.anarazel.de

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> There's also
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jabiru&dt=2022-03-22%2022%3A25%3A26
> that started failing with
> ../../preproc/ecpg --regression -I./../../include -I. -o test1.c test1.pgc
> test1.pgc:12: ERROR: syntax error at or near "int"
> with this commit.

Yeah, I was just scratching my head about that.  It's not clear how
that could be related; but jabiru doesn't seem to have a history of
random failures, so maybe it's real.

            regards, tom lane

On 3/22/22 18:31, Tom Lane wrote:
> I wrote:
>> That patch is 0-for-three on my buildfarm animals.
> ... the reason being that they use -Werror, and this patch spews
> a bunch of warnings.  This is not acceptable, especially not in
> the middle of a CF when other people are trying to get work done.
> Please revert.
>
>             



reverted.


cheers


andrew

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

I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> There's also
>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jabiru&dt=2022-03-22%2022%3A25%3A26
>> that started failing with
>> ../../preproc/ecpg --regression -I./../../include -I. -o test1.c test1.pgc
>> test1.pgc:12: ERROR: syntax error at or near "int"
>> with this commit.

> Yeah, I was just scratching my head about that.

I have a possibly-far-fetched theory: the ecpg grammar builder has
certainly never been validated against a backend grammar that
contains unused rules or unused nonterminals.  Maybe it generates
a subtly incorrect .y file in such cases, and on this particular
platform that results in bad code generated by bison, and ensuing
bogus syntax errors.

The lack of other failures weakens this theory.  Notably, I failed
to duplicate the problem on florican's host, which has the same
nominal bison version 3.7.6.  But it wouldn't surprise me a bit
to find that OpenBSD is carrying some private patches for their
build, so maybe that matters?

In any case, I think it's a bit pointless to chase these issues
with respect to this intermediate state of the JSON patch.
Let's merge in the next step, get to a state that does not
generate build warnings, and see what happens then.

            regards, tom lane

At least 0002-SQL-JSON-constructors-v64.patch has an issue with nodes,
per COPY_PARSE_PLAN_TREES.

+ERROR:  unrecognized node type: 157

On 3/23/22 08:24, Justin Pryzby wrote:
> At least 0002-SQL-JSON-constructors-v64.patch has an issue with nodes,
> per COPY_PARSE_PLAN_TREES.
>
> +ERROR:  unrecognized node type: 157



I just tried to reproduce this and was unable to.  Ubuntu 20.04, gcc 9.4.0.


the build was done with CPPFLAGS=-DCOPY_PARSE_PLAN_TREES. To be on the
safe side I took ccache out of the equation.


Can you give me any more details?


cheers


andrew


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

On 3/23/22 15:49, Andrew Dunstan wrote:
> On 3/23/22 08:24, Justin Pryzby wrote:
>> At least 0002-SQL-JSON-constructors-v64.patch has an issue with nodes,
>> per COPY_PARSE_PLAN_TREES.
>>
>> +ERROR:  unrecognized node type: 157
>
>
> I just tried to reproduce this and was unable to.  Ubuntu 20.04, gcc 9.4.0.
>
>
> the build was done with CPPFLAGS=-DCOPY_PARSE_PLAN_TREES. To be on the
> safe side I took ccache out of the equation.
>
>
> Can you give me any more details?
>
>

I have reproduced similar errors from patch 4 in the series.


cheers


andrew


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

On Wed, Mar 23, 2022 at 03:49:17PM -0400, Andrew Dunstan wrote:
> 
> On 3/23/22 08:24, Justin Pryzby wrote:
> > At least 0002-SQL-JSON-constructors-v64.patch has an issue with nodes,
> > per COPY_PARSE_PLAN_TREES.
> >
> > +ERROR:  unrecognized node type: 157
> 
> I just tried to reproduce this and was unable to.  Ubuntu 20.04, gcc 9.4.0.
> 
> the build was done with CPPFLAGS=-DCOPY_PARSE_PLAN_TREES. To be on the
> safe side I took ccache out of the equation.
> 
> Can you give me any more details?

Sorry, the issue was actually with 

#define RAW_EXPRESSION_COVERAGE_TEST

make check is enough to see it:

@@ -6,181 +6,78 @@
 (1 row)
 
 SELECT JSON_OBJECT(RETURNING json);
- json_object 
--------------
- {}
-(1 row)
-
+ERROR:  unrecognized node type: 157
[...]

On 3/23/22 16:56, Justin Pryzby wrote:
> On Wed, Mar 23, 2022 at 03:49:17PM -0400, Andrew Dunstan wrote:
>> On 3/23/22 08:24, Justin Pryzby wrote:
>>> At least 0002-SQL-JSON-constructors-v64.patch has an issue with nodes,
>>> per COPY_PARSE_PLAN_TREES.
>>>
>>> +ERROR:  unrecognized node type: 157
>> I just tried to reproduce this and was unable to.  Ubuntu 20.04, gcc 9.4.0.
>>
>> the build was done with CPPFLAGS=-DCOPY_PARSE_PLAN_TREES. To be on the
>> safe side I took ccache out of the equation.
>>
>> Can you give me any more details?
> Sorry, the issue was actually with 
>
> #define RAW_EXPRESSION_COVERAGE_TEST
>
> make check is enough to see it:
>
> @@ -6,181 +6,78 @@
>  (1 row)
>  
>  SELECT JSON_OBJECT(RETURNING json);
> - json_object 
> --------------
> - {}
> -(1 row)
> -
> +ERROR:  unrecognized node type: 157
> [...]


Here's a new patch set that fixes this and several similar issues.
Thanks to my colleague  Masahiko Sawada for providing fixes for some of
these at very short notice.


I wonder if we should add these compile flags to the cfbot's setup?


cheers


andrew


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

Hi,

On 2022-03-24 18:51:30 -0400, Andrew Dunstan wrote:
> I wonder if we should add these compile flags to the cfbot's setup?

Yes, I think we should. There's a bit of discussion of that in and below
https://postgr.es/m/20220213051937.GO31460%40telsasoft.com - that veered a bit
of course, so I haven't done anything about it yet.  Perhaps one build
COPY_PARSE_PLAN_TREES and RAW_EXPRESSION_COVERAGE_TEST another
WRITE_READ_PARSE_PLAN_TREES?  We should add the slower to the macos build,
that's plenty fast and I'm intending to slow the linux test by using ubsan,
which works better on linux.

Greetings,

Andres Freund

I wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> There's also
>>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jabiru&dt=2022-03-22%2022%3A25%3A26
>>> that started failing with
>>> ../../preproc/ecpg --regression -I./../../include -I. -o test1.c test1.pgc
>>> test1.pgc:12: ERROR: syntax error at or near "int"
>>> with this commit.

>> Yeah, I was just scratching my head about that.

This problem came back as soon as we de-reverted that patch :-(.
So much for my guess about unused rules.

What's worse, I'm unable to replicate the failure on an OpenBSD 7.0
system here.  So there's something odd about jabiru's build
environment; but what?

            regards, tom lane

> On Mar 27, 2022, at 7:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
>>> Andres Freund <andres@anarazel.de> writes:
>>>> There's also
>>>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jabiru&dt=2022-03-22%2022%3A25%3A26
>>>> that started failing with
>>>> ../../preproc/ecpg --regression -I./../../include -I. -o test1.c test1.pgc
>>>> test1.pgc:12: ERROR: syntax error at or near "int"
>>>> with this commit.
>
>>> Yeah, I was just scratching my head about that.
>
> This problem came back as soon as we de-reverted that patch :-(.
> So much for my guess about unused rules.
>
> What's worse, I'm unable to replicate the failure on an OpenBSD 7.0
> system here.  So there's something odd about jabiru's build
> environment; but what?
>

Very odd. How did it pick up just this commit and not the following one which was pushed at the same time?

Cheers

Andrew

Hi,

On 2022-03-27 20:21:05 -0400, Andrew Dunstan wrote:
> Very odd. How did it pick up just this commit and not the following one which was pushed at the same time?

The first recent failing run was with
f4fb45d15c Sun Mar 27 21:03:34 2022 UTC  SQL/JSON constructors
f79b803dcc Sun Mar 27 21:03:33 2022 UTC  Common SQL/JSON clauses
b64c3bd62e Sun Mar 27 20:26:40 2022 UTC  Remove more unused module imports from TAP tests

And then a second failure with:
cc7401d5ca Sun Mar 27 22:32:40 2022 UTC  Fix up compiler warnings/errors from f4fb45d15.

Which is what I'd expect?

Greetings,

Andres Freund

On 3/27/22 20:50, Andres Freund wrote:
> Hi,
>
> On 2022-03-27 20:21:05 -0400, Andrew Dunstan wrote:
>> Very odd. How did it pick up just this commit and not the following one which was pushed at the same time?
> The first recent failing run was with
> f4fb45d15c Sun Mar 27 21:03:34 2022 UTC  SQL/JSON constructors
> f79b803dcc Sun Mar 27 21:03:33 2022 UTC  Common SQL/JSON clauses
> b64c3bd62e Sun Mar 27 20:26:40 2022 UTC  Remove more unused module imports from TAP tests
>
> And then a second failure with:
> cc7401d5ca Sun Mar 27 22:32:40 2022 UTC  Fix up compiler warnings/errors from f4fb45d15.
>
> Which is what I'd expect?
>


Yes, sorry, The url was for the previous commit, not today's. My mistake.


I'll look into it tomorrow, too tired tonight to be very productive.


cheers


andrew


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

On 3/27/22 19:14, Tom Lane wrote:
> I wrote:
>>> Andres Freund <andres@anarazel.de> writes:
>>>> There's also
>>>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jabiru&dt=2022-03-22%2022%3A25%3A26
>>>> that started failing with
>>>> ../../preproc/ecpg --regression -I./../../include -I. -o test1.c test1.pgc
>>>> test1.pgc:12: ERROR: syntax error at or near "int"
>>>> with this commit.
>>> Yeah, I was just scratching my head about that.
> This problem came back as soon as we de-reverted that patch :-(.
> So much for my guess about unused rules.
>
> What's worse, I'm unable to replicate the failure on an OpenBSD 7.0
> system here.  So there's something odd about jabiru's build
> environment; but what?
>
>             



It's hard to see how this could be caused by the OS environment. Maybe a
flaky bison/flex? I'm going to be pretty reluctant to revert this based
on this error.


cheers


andrew


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

Andrew Dunstan <andrew@dunslane.net> writes:
> On 3/27/22 19:14, Tom Lane wrote:
>> What's worse, I'm unable to replicate the failure on an OpenBSD 7.0
>> system here.  So there's something odd about jabiru's build
>> environment; but what?

> It's hard to see how this could be caused by the OS environment. Maybe a
> flaky bison/flex? I'm going to be pretty reluctant to revert this based
> on this error.

No, I wouldn't recommend reverting.  Perhaps if we dig down and find
something reproducible here, we could fix it --- but right now,
given my failure to reproduce, I think there's just something broken
on jabiru.

            regards, tom lane

On 3/28/22 09:35, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 3/27/22 19:14, Tom Lane wrote:
>>> What's worse, I'm unable to replicate the failure on an OpenBSD 7.0
>>> system here.  So there's something odd about jabiru's build
>>> environment; but what?
>> It's hard to see how this could be caused by the OS environment. Maybe a
>> flaky bison/flex? I'm going to be pretty reluctant to revert this based
>> on this error.
> No, I wouldn't recommend reverting.  Perhaps if we dig down and find
> something reproducible here, we could fix it --- but right now,
> given my failure to reproduce, I think there's just something broken
> on jabiru.
>
>             



Yeah. I have just duplicated your non-replication on a fresh instance.


Nikola Ivanov, can you give us any assistance or give us access to the
machine?


cheers


andrew


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

On 3/28/22 11:05, Nikola Ivanov wrote:
> Hi Andrew,
>
> Let me know check what can I do with the access. I will get back to
> you in an hour.


Thanks for you help and prompt response.

In the first instance we'd like to know what might be different about
jabiru from the openbsd7/clang11 instances Tom and I have just
successfully tested on. In the last resort we might need to run ecpg
under a debugger on jabiru to see why it's failing there and not
elsewhere. To set up for that run the buildfarm script with --keepall.


cheers


andrew


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

Hi,

On 2022-03-28 18:05:19 +0300, Nikola Ivanov wrote:
> Let me know check what can I do with the access. I will get back to you in
> an hour.

Perhaps you can temporarily enable keep_error_builds, and send in
src/interfaces/ecpg/preproc/c_kwlist_d.h
src/interfaces/ecpg/preproc/pgc.c
src/interfaces/ecpg/preproc/preproc.h
src/interfaces/ecpg/preproc/ecpg_kwlist_d.h
src/interfaces/ecpg/preproc/preproc.c
from the failed build directory? It seems something there have to differ.

Regards,

Andres

On 3/28/22 14:31, Nikola Ivanov wrote:
> Hi Andreas,
>
> Archive with the files is attached.



That didn't help, there are no differences that matter (just #line
directives as I did a vpath build). :-(


cheers


andrew


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

Hi,

On 2022-03-28 14:57:20 -0400, Andrew Dunstan wrote:
> That didn't help, there are no differences that matter (just #line
> directives as I did a vpath build). :-(

Yea. I didn't see any differences when comparing to a non-vpath build that
runs tests successfully. Pretty weird.

Nikola, unless remote access turns out to be possible for one of us, could you
perhaps try to build interactively and see whether it reproduces there?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2022-03-28 14:57:20 -0400, Andrew Dunstan wrote:
>> That didn't help, there are no differences that matter (just #line
>> directives as I did a vpath build). :-(

> Yea. I didn't see any differences when comparing to a non-vpath build that
> runs tests successfully. Pretty weird.

Unsurprisingly, these files match what I built, too.  So the problem
is downstream of the flex/bison runs.  Baffling :-(

            regards, tom lane

... even more baffling: jabiru went green after the IS JSON patch.

            regards, tom lane

> On Mar 28, 2022, at 7:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> ... even more baffling: jabiru went green after the IS JSON patch.
>
>

Yeah, bizarre. Let’s see if I can upset that tomorrow with the next patch :-)

cheers

andrew

On 2022-03-28 19:25:51 -0400, Tom Lane wrote:
> ... even more baffling: jabiru went green after the IS JSON patch.

Broken ccache contents somehow?

I see several commits referencing this entry. Can we mark it committed
or are there still chunks of commits to go?

On 3/31/22 15:54, Greg Stark wrote:
> I see several commits referencing this entry. Can we mark it committed
> or are there still chunks of commits to go?



No code chunks left, only a documentation patch which should land
tomorrow or Saturday.


I am also planning on committing the JSON_TABLE patches before feature
freeze (April 7). They depend on this set.


cheers


andrew

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

On Thu, Mar 31, 2022 at 04:25:58PM -0400, Andrew Dunstan wrote:
> No code chunks left, only a documentation patch which should land

Documentation review for a6baa4bad.

> Construct a JSON the provided strings:

a JSON what ?
*from* the provided strings ?

> Construct a JSON from the provided values various types:

should say "a JSON scalar" ?
*of* various types ?

> Construct a JSON object from the provided key/value pairs of various types:

For comparison, that one looks ok.

+      <function>JSON_EXISTS</function> function checks whether the provided
+   <function>JSON_VALUE</function> function extracts a value from the provided
+   <function>JSON_QUERY</function> function extracts an <acronym>SQL/JSON</acronym>
+      <function>JSON_TABLE</function> function queries <acronym>JSON</acronym> data
+     <function>JSON_TABLE</function> uses the
+      <function>JSON_SERIALIZE</function> function transforms a SQL/JSON value

I think all these should all begin with "THE >...< function ...", like the
others do.

+To use other types, you must create the <literal>CAST</literal> from <type>json</type> for this type.
=> create a cast from json to this type.

+Values can be null, but not keys.
I think it's clearer to say "..but keys cannot."

+          For any scalar other than a number or a Boolean the text

Boolean COMMA the text

+     The path name must be unique and cannot coincide with column names.
Maybe say "name must be unique and distinct from the column names."

+      ... If you specify a <command>GROUP BY</command>
+      or an <command>ORDER BY</command> clause, this function returns a separate JSON object
+      for each table row.

"for each table row" sounds inaccurate or imprecise.  The SELECT docs say this:
| GROUP BY will condense into a single row all selected rows that share the same values for the grouped expressions

BTW, the documentation references look a little like OIDs...
Does someone already have an SNMP-based doc browser ?
| For details, see Section 9.16.3.4.2.

On 4/8/22 08:02, Justin Pryzby wrote:
> On Thu, Mar 31, 2022 at 04:25:58PM -0400, Andrew Dunstan wrote:
>> No code chunks left, only a documentation patch which should land
> Documentation review for a6baa4bad.
>
>> Construct a JSON the provided strings:
> a JSON what ?
> *from* the provided strings ?
>
>> Construct a JSON from the provided values various types:
> should say "a JSON scalar" ?
> *of* various types ?
>
>> Construct a JSON object from the provided key/value pairs of various types:
> For comparison, that one looks ok.
>
> +      <function>JSON_EXISTS</function> function checks whether the provided
> +   <function>JSON_VALUE</function> function extracts a value from the provided
> +   <function>JSON_QUERY</function> function extracts an <acronym>SQL/JSON</acronym>
> +      <function>JSON_TABLE</function> function queries <acronym>JSON</acronym> data
> +     <function>JSON_TABLE</function> uses the
> +      <function>JSON_SERIALIZE</function> function transforms a SQL/JSON value
>
> I think all these should all begin with "THE >...< function ...", like the
> others do.
>
> +To use other types, you must create the <literal>CAST</literal> from <type>json</type> for this type.
> => create a cast from json to this type.
>
> +Values can be null, but not keys.
> I think it's clearer to say "..but keys cannot."
>
> +          For any scalar other than a number or a Boolean the text
>
> Boolean COMMA the text
>
> +     The path name must be unique and cannot coincide with column names.
> Maybe say "name must be unique and distinct from the column names."
>
> +      ... If you specify a <command>GROUP BY</command>
> +      or an <command>ORDER BY</command> clause, this function returns a separate JSON object
> +      for each table row.
>
> "for each table row" sounds inaccurate or imprecise.  The SELECT docs say this:
> | GROUP BY will condense into a single row all selected rows that share the same values for the grouped expressions
>
> BTW, the documentation references look a little like OIDs...
> Does someone already have an SNMP-based doc browser ?
> | For details, see Section 9.16.3.4.2.



Many thanks, useful.

I already had a couple of these items on my list but I ran out of time
before tiredness overcame me last night.

I'm planning on removing some of that stuff that generates the last
complaint if I can do it without too much violence.


cheers


andrew

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

On 2022-04-08 Fr 08:15, Andrew Dunstan wrote:
> On 4/8/22 08:02, Justin Pryzby wrote:
>> On Thu, Mar 31, 2022 at 04:25:58PM -0400, Andrew Dunstan wrote:
>>> No code chunks left, only a documentation patch which should land
>> Documentation review for a6baa4bad.
>>
>>> Construct a JSON the provided strings:
>> a JSON what ?
>> *from* the provided strings ?
>>
>>> Construct a JSON from the provided values various types:
>> should say "a JSON scalar" ?
>> *of* various types ?
>>
>>> Construct a JSON object from the provided key/value pairs of various types:
>> For comparison, that one looks ok.
>>
>> +      <function>JSON_EXISTS</function> function checks whether the provided
>> +   <function>JSON_VALUE</function> function extracts a value from the provided
>> +   <function>JSON_QUERY</function> function extracts an <acronym>SQL/JSON</acronym>
>> +      <function>JSON_TABLE</function> function queries <acronym>JSON</acronym> data
>> +     <function>JSON_TABLE</function> uses the
>> +      <function>JSON_SERIALIZE</function> function transforms a SQL/JSON value
>>
>> I think all these should all begin with "THE >...< function ...", like the
>> others do.
>>
>> +To use other types, you must create the <literal>CAST</literal> from <type>json</type> for this type.
>> => create a cast from json to this type.
>>
>> +Values can be null, but not keys.
>> I think it's clearer to say "..but keys cannot."
>>
>> +          For any scalar other than a number or a Boolean the text
>>
>> Boolean COMMA the text
>>
>> +     The path name must be unique and cannot coincide with column names.
>> Maybe say "name must be unique and distinct from the column names."
>>
>> +      ... If you specify a <command>GROUP BY</command>
>> +      or an <command>ORDER BY</command> clause, this function returns a separate JSON object
>> +      for each table row.
>>
>> "for each table row" sounds inaccurate or imprecise.  The SELECT docs say this:
>> | GROUP BY will condense into a single row all selected rows that share the same values for the grouped expressions
>>
>> BTW, the documentation references look a little like OIDs...
>> Does someone already have an SNMP-based doc browser ?
>> | For details, see Section 9.16.3.4.2.
>
>
> Many thanks, useful.
>
> I already had a couple of these items on my list but I ran out of time
> before tiredness overcame me last night.
>
> I'm planning on removing some of that stuff that generates the last
> complaint if I can do it without too much violence.
>
>

I have attended to most of these. I just removed the GROUP BY sentence,
I don't think it added anything useful. We already refer users to the
aggregates page.


I will deal with the structural issues soon.


cheers


andrew

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

On Mon, Apr 11, 2022 at 11:54:11AM -0400, Andrew Dunstan wrote:
> >> BTW, the documentation references look a little like OIDs...
> >> Does someone already have an SNMP-based doc browser ?
> >> | For details, see Section 9.16.3.4.2.
> >
> > I already had a couple of these items on my list but I ran out of time
> > before tiredness overcame me last night.
> >
> > I'm planning on removing some of that stuff that generates the last
> > complaint if I can do it without too much violence.
> 
> I will deal with the structural issues soon.

I didn't actually intend this as a complaint.  The ability for a reference to
be specific and granular is good.  So there may be reasons to change the
structure, but my comment is only about a +0.1.

Justin Pryzby <pryzby@telsasoft.com> writes:
> On Mon, Apr 11, 2022 at 11:54:11AM -0400, Andrew Dunstan wrote:
>> I will deal with the structural issues soon.

> I didn't actually intend this as a complaint.  The ability for a reference to
> be specific and granular is good.  So there may be reasons to change the
> structure, but my comment is only about a +0.1.

I was going to complain about it on the grounds that it looks
approximately nothing like other sections of func.sgml.  The layout
of the rest of that file is hard-won presentation knowledge and
should not be lightly ignored.

            regards, tom lane

Hello hackers,
On branch REL_15_STABLE the following query: SELECT 
JSON_SERIALIZE('{"a":1}' RETURNING jsonb);

produces SIGSEGV for me:
#0  getJsonbOffset (index=39920251, jc=0x563cc5601d5c) at 
jsonb_util.c:148
148                     offset += JBE_OFFLENFLD(jc->children[i]);
(gdb) bt
#0  getJsonbOffset (index=39920251, jc=0x563cc5601d5c) at 
jsonb_util.c:148
#1  JsonbIteratorNext (it=0x7ffdaff752c8, val=0x7ffdaff752d0, 
skipNested=false) at jsonb_util.c:933
#2  0x0000563cc3e6934b in JsonbToCStringWorker (out=0x563cc55fe9a0, 
in=<optimized out>, estimated_len=<optimized out>, indent=false) at 
jsonb.c:496
#3  0x0000563cc3f35c68 in FunctionCall1Coll (arg1=<optimized out>, 
collation=0, flinfo=0x563cc55fb8a0) at fmgr.c:1124
#4  OutputFunctionCall (flinfo=0x563cc55fb8a0, val=<optimized out>) at 
fmgr.c:1561
#5  0x0000563cc3a8ebc5 in printtup (slot=0x563cc55faed8, 
self=0x563cc5602758) at printtup.c:357
#6  0x0000563cc3c51917 in ExecutePlan (execute_once=<optimized out>, 
dest=0x563cc5602758, direction=<optimized out>, numberTuples=0, 
sendTuples=<optimized out>, operation=CMD_SELECT, 
use_parallel_mode=<optimized out>, planstate=0x563cc55fabb8,
     estate=0x563cc55fa980) at execMain.c:1667
#7  standard_ExecutorRun (queryDesc=0x563cc5553e60, direction=<optimized 
out>, count=0, execute_once=<optimized out>) at execMain.c:363
#8  0x0000563cc3df741f in PortalRunSelect (portal=0x563cc55a3c40, 
forward=<optimized out>, count=0, dest=<optimized out>) at pquery.c:924
#9  0x0000563cc3df8b81 in PortalRun (portal=portal@entry=0x563cc55a3c40, 
count=count@entry=9223372036854775807, isTopLevel=isTopLevel@entry=true, 
run_once=run_once@entry=true, dest=dest@entry=0x563cc5602758, 
altdest=altdest@entry=0x563cc5602758,
     qc=0x7ffdaff75650) at pquery.c:768
#10 0x0000563cc3df498d in exec_simple_query (query_string=0x563cc5531f00 
"SELECT JSON_SERIALIZE('{\"a\":1}' RETURNING jsonb);") at 
postgres.c:1250
#11 0x0000563cc3df651a in PostgresMain (dbname=<optimized out>, 
username=<optimized out>) at postgres.c:4558
#12 0x0000563cc3d5b5e7 in BackendRun (port=<optimized out>, 
port=<optimized out>) at postmaster.c:4504
#13 BackendStartup (port=<optimized out>) at postmaster.c:4232
#14 ServerLoop () at postmaster.c:1806
#15 0x0000563cc3d5c6da in PostmasterMain (argc=argc@entry=3, 
argv=argv@entry=0x563cc552c6d0) at postmaster.c:1478
#16 0x0000563cc3a7b550 in main (argc=3, argv=0x563cc552c6d0) at 
main.c:202

The first bad commit is 606948b058dc16bce494270eea577011a602810e

Andrew Dunstan писал 2022-04-01 03:25:
> On 3/31/22 15:54, Greg Stark wrote:
>> I see several commits referencing this entry. Can we mark it committed
>> or are there still chunks of commits to go?
> 
> 
> 
> No code chunks left, only a documentation patch which should land
> tomorrow or Saturday.
> 
> 
> I am also planning on committing the JSON_TABLE patches before feature
> freeze (April 7). They depend on this set.
> 
> 
> cheers
> 
> 
> andrew
> 
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com

On 2022-07-07 Th 12:38, a.kozhemyakin@postgrespro.ru wrote:
> Hello hackers,
> On branch REL_15_STABLE the following query: SELECT
> JSON_SERIALIZE('{"a":1}' RETURNING jsonb);
>
> produces SIGSEGV for me:
> #0  getJsonbOffset (index=39920251, jc=0x563cc5601d5c) at
> jsonb_util.c:148
> 148                     offset += JBE_OFFLENFLD(jc->children[i]);
> (gdb) bt
> #0  getJsonbOffset (index=39920251, jc=0x563cc5601d5c) at
> jsonb_util.c:148
> #1  JsonbIteratorNext (it=0x7ffdaff752c8, val=0x7ffdaff752d0,
> skipNested=false) at jsonb_util.c:933
> #2  0x0000563cc3e6934b in JsonbToCStringWorker (out=0x563cc55fe9a0,
> in=<optimized out>, estimated_len=<optimized out>, indent=false) at
> jsonb.c:496
> #3  0x0000563cc3f35c68 in FunctionCall1Coll (arg1=<optimized out>,
> collation=0, flinfo=0x563cc55fb8a0) at fmgr.c:1124
> #4  OutputFunctionCall (flinfo=0x563cc55fb8a0, val=<optimized out>) at
> fmgr.c:1561
> #5  0x0000563cc3a8ebc5 in printtup (slot=0x563cc55faed8,
> self=0x563cc5602758) at printtup.c:357
> #6  0x0000563cc3c51917 in ExecutePlan (execute_once=<optimized out>,
> dest=0x563cc5602758, direction=<optimized out>, numberTuples=0,
> sendTuples=<optimized out>, operation=CMD_SELECT,
> use_parallel_mode=<optimized out>, planstate=0x563cc55fabb8,
>     estate=0x563cc55fa980) at execMain.c:1667
> #7  standard_ExecutorRun (queryDesc=0x563cc5553e60,
> direction=<optimized out>, count=0, execute_once=<optimized out>) at
> execMain.c:363
> #8  0x0000563cc3df741f in PortalRunSelect (portal=0x563cc55a3c40,
> forward=<optimized out>, count=0, dest=<optimized out>) at pquery.c:924
> #9  0x0000563cc3df8b81 in PortalRun
> (portal=portal@entry=0x563cc55a3c40,
> count=count@entry=9223372036854775807,
> isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true,
> dest=dest@entry=0x563cc5602758, altdest=altdest@entry=0x563cc5602758,
>     qc=0x7ffdaff75650) at pquery.c:768
> #10 0x0000563cc3df498d in exec_simple_query
> (query_string=0x563cc5531f00 "SELECT JSON_SERIALIZE('{\"a\":1}'
> RETURNING jsonb);") at postgres.c:1250
> #11 0x0000563cc3df651a in PostgresMain (dbname=<optimized out>,
> username=<optimized out>) at postgres.c:4558
> #12 0x0000563cc3d5b5e7 in BackendRun (port=<optimized out>,
> port=<optimized out>) at postmaster.c:4504
> #13 BackendStartup (port=<optimized out>) at postmaster.c:4232
> #14 ServerLoop () at postmaster.c:1806
> #15 0x0000563cc3d5c6da in PostmasterMain (argc=argc@entry=3,
> argv=argv@entry=0x563cc552c6d0) at postmaster.c:1478
> #16 0x0000563cc3a7b550 in main (argc=3, argv=0x563cc552c6d0) at
> main.c:202
>
> The first bad commit is 606948b058dc16bce494270eea577011a602810e



Thanks for the report. Looking into it.


cheers


andrew


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

On 2022-07-07 Th 14:35, Andrew Dunstan wrote:
> On 2022-07-07 Th 12:38, a.kozhemyakin@postgrespro.ru wrote:
>> Hello hackers,
>> On branch REL_15_STABLE the following query: SELECT
>> JSON_SERIALIZE('{"a":1}' RETURNING jsonb);
>>
>> produces SIGSEGV for me:





You're not supposed to be able to do that - see the docs on
json_serialize. I've committed a patch that detects this and causes an
error instead of a segfault.


cheers


andrew


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