Thread: [HACKERS] SQL/JSON in PostgreSQL

Hi Oleg,

On 2/28/17 2:55 PM, Pavel Stehule wrote:
> 2017-02-28 20:08 GMT+01:00 Oleg Bartunov <obartunov@gmail.com
> 
>     Attached patch is an implementation of SQL/JSON data model from
>     SQL-2016 standard (ISO/IEC 9075-2:2016(E)), which was published
>     2016-12-15 and is available only for purchase from ISO web site
>     (https://www.iso.org/standard/63556.html
>     <https://www.iso.org/standard/63556.html>). Unfortunately I didn't
>     find any public sources of the standard or any preview documents,
>     but Oracle implementation of json support in 12c release 2 is very
>     close
>     (http://docs.oracle.com/database/122/ADJSN/json-in-oracle-database.htm
>     <http://docs.oracle.com/database/122/ADJSN/json-in-oracle-database.htm>),
>     also we used https://livesql.oracle.com/  to understand some details.

<...>

> This is last commitfest for current release cycle - are you sure, so is
> good idea to push all mentioned features?

Implementing standards is always a goal of the PostgreSQL community, but
this is a very large patch arriving very late in the release cycle with
no prior discussion.

That the patch proposed follows a standard which will not be available
to the majority of reviewers is very worrisome, let alone the sheer
size.  While much of the code is new, I see many changes to core data
structures that could very easily be destabilizing.

I propose we move this patch to the 2017-07 CF so further development
and review can be done without haste and as the standard becomes more
accessible.

Regards,
-- 
-David
david@pgmasters.net

Hi,

On 2017-03-07 12:21:59 +0300, Oleg Bartunov wrote:
> On 2017-03-03 15:49:38 -0500, David Steele wrote:
> > I propose we move this patch to the 2017-07 CF so further development
> > and review can be done without haste and as the standard becomes more
> > accessible.

+1


> I wanted to have one more  good feature in 10 and let postgres be on par
> with other competitors.  SQL/JSON adds many interesting features and users
> will be dissapointed if we postpone it for next two years.   Let's wait for
> reviewers, probably they will find the patch is not very  intrusive.

I think it's way too late to late for a patch of this size for 10. And I
don't think it's fair to a lot of other patches of significant size that
have been submitted way earlier, that also need reviewing resources, to
say that we can just see whether it'll get the required resources.


> We have a plenty of time and we dedicate one full-time developer for
> this project.

How about having that, and perhaps others, developer participate in
reviewing patches and getting to the bottom of the commitfest?  Should
we end up being done early, we can look at this patch...  There's not
been review activity corresponding to the amount of submissions from
pgpro...

- Andres

Hi,

about the datetime issue: as far as I know, JSON does not define a 
serialization format for dates and timestamps.

On the other hand, YAML (as a superset of JSON) already supports a 
language-independent date(time) serialization format 
(http://yaml.org/type/timestamp.html).

I haven't had a glance into the SQL/JSON standard yet and a quick search 
didn't reveal anything. However, reading your test case here 
https://github.com/postgrespro/sqljson/blob/5a8a241/src/test/regress/sql/sql_json.sql#L411 
it seems as if you intend to parse all strings in the form of 
"YYYY-MM-DD" as dates. This is problematic in case a string happens to 
look like this but is not intended to be a date.

Just for the sake of completeness: YAML solves this issue by omitting 
the quotation marks around the date string (just as JSON integers have 
no quotations marks around them).

Regards,
Sven

On 3/7/17 11:38 AM, Andres Freund wrote:

<...>

>> We have a plenty of time and we dedicate one full-time developer for
>> this project.
>
> How about having that, and perhaps others, developer participate in
> reviewing patches and getting to the bottom of the commitfest?  Should
> we end up being done early, we can look at this patch...  There's not
> been review activity corresponding to the amount of submissions from
> pgpro...

This patch has been moved to CF 2017-07.

-- 
-David
david@pgmasters.net

On Tue, Mar 7, 2017 at 2:38 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2017-03-07 12:21:59 +0300, Oleg Bartunov wrote:
>> On 2017-03-03 15:49:38 -0500, David Steele wrote:
>> > I propose we move this patch to the 2017-07 CF so further development
>> > and review can be done without haste and as the standard becomes more
>> > accessible.
>
> +1

I agree that this should not go into v10.  February 28th is not the
right time for a large, never-before-seen patch to show up with
expectations of getting committed for the current cycle.

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

On 08.03.2017 20:48, Peter van Hardenberg wrote:
> Small point of order: YAML is not strictly a super-set of JSON.

I haven't read the whole standard, but from what I can see the standard 
considers JSON an official subset of itself: 
http://www.yaml.org/spec/1.2/spec.html

Regards,
Sven

On Thu, Mar 9, 2017 at 12:48 PM, Sven R. Kunze <srkunze@mail.de> wrote:
> On 08.03.2017 20:48, Peter van Hardenberg wrote:
>>
>> Small point of order: YAML is not strictly a super-set of JSON.
>
> I haven't read the whole standard, but from what I can see the standard
> considers JSON an official subset of itself:
> http://www.yaml.org/spec/1.2/spec.html

But there's apparent sophistry, like this, in that spec:

SON's RFC4627 requires that mappings keys merely “SHOULD” be unique,
while YAML insists they “MUST” be. Technically, YAML therefore
complies with the JSON spec, choosing to treat duplicates as an error.
In practice, since JSON is silent on the semantics of such duplicates,
the only portable JSON files are those with unique keys, which are
therefore valid YAML files.

I don't see how YAML can impose a stronger requirement than JSON and
yet claim to be a superset; a JSON document that doesn't meet that
requirement will be legal (if stupid) as JSON but illegal as YAML.

Also, even if the superset thing were true on a theoretical plane, I'm
not sure it would do us much good in practice.  If we start using
YAML-specific constructs, we won't have valid JSON any more.  If we
use only things that are legal in JSON, YAML's irrelevant.

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

On Wed, Mar 8, 2017 at 11:48 AM, Peter van Hardenberg <pvh@pvh.ca> wrote:

Small point of order: YAML is not strictly a super-set of JSON.

Editorializing slightly, I have not seen much interest in the world for YAML support though I'd be interested in evidence to the contrary.

The world of configuration management seems to for some reason run off YAML, but that's the only places I've seen it recently (ansible, puppet etc).

That said if we're introducing something new, it's usually better to copy from another format than to invite your own.

On 09.03.2017 18:58, Robert Haas wrote:
> Also, even if the superset thing were true on a theoretical plane, I'm
> not sure it would do us much good in practice.  If we start using
> YAML-specific constructs, we won't have valid JSON any more.  If we
> use only things that are legal in JSON, YAML's irrelevant.

That's true. I just wanted to share my view of the "date guessing" part 
of pgpro's commits.
I don't have a good solution for it either, I can only tell that where I 
work we do have same issues: either we guess by looking at the string 
value or we know that "this particular key" must be a date.
Unsatisfied with either solution, we tend to use YAML for our APIs if 
possible.

Regards,
Sven

On 09.03.2017 19:50, Peter van Hardenberg wrote:
> Anecdotally, we just stored dates as strings and used a convention 
> (key ends in "_at", I believe) to interpret them. The lack of support 
> for dates in JSON is well-known, universally decried... and not a 
> problem the PostgreSQL community can fix.

I completely agree here.

Regards,
Sven

On 09/03/17 19:50, Peter van Hardenberg wrote:
> Anecdotally, we just stored dates as strings and used a convention (key
> ends in "_at", I believe) to interpret them. The lack of support for
> dates in JSON is well-known, universally decried... and not a problem
> the PostgreSQL community can fix.
> 

The original complain was about JSON_VALUE extracting date but I don't
understand why there is problem with that, the SQL/JSON defines that
behavior. The RETURNING clause there is more or less just shorthand for
casting with some advanced options.

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On 10.03.2017 05:07, Petr Jelinek wrote:
> The original complain was about JSON_VALUE extracting date but I don't
> understand why there is problem with that, the SQL/JSON defines that
> behavior. The RETURNING clause there is more or less just shorthand for
> casting with some advanced options.

Thanks for clarifying. I mistook it as if JSON_VALUE itself returns a 
date value.

Sven

On 03/09/2017 10:12 AM, Sven R. Kunze wrote:
> On 08.03.2017 20:52, Magnus Hagander wrote:
>> On Wed, Mar 8, 2017 at 11:48 AM, Peter van Hardenberg <pvh@pvh.ca
>> <mailto:pvh@pvh.ca>> wrote:
>>
>>     Small point of order: YAML is not strictly a super-set of JSON.
>>
>>     Editorializing slightly, I have not seen much interest in the
>>     world for YAML support though I'd be interested in evidence to the
>>     contrary.
>>
>>
>> The world of configuration management seems to for some reason run off
>> YAML, but that's the only places I've seen it recently (ansible,
>> puppet etc).
> 
> SaltStack uses YAML for their tools, too. I personally can empathize
> with them (as a user of configuration management) about this as writing
> JSON would be nightmare with all the quoting, commas, curly braces etc.
> But that's my own preference maybe.
> 
> (Btw. does "run off" mean like or avoid? At least my dictionaries tend
> to the latter.)

Yes, but automated tools can easily convert between JSON and
newline-delimited YAML and back.

-- 
Josh Berkus
Containers & Databases Oh My!

On Tue, Mar 07, 2017 at 10:43:16PM +0100, Sven R. Kunze wrote:
> about the datetime issue: as far as I know, JSON does not define a
> serialization format for dates and timestamps.

Use strings in ISO 8601 format, with or without fractional seconds, and
maybe with 5-digit years.

> On the other hand, YAML (as a superset of JSON) already supports a
> language-independent date(time) serialization format
> (http://yaml.org/type/timestamp.html).

But YAML isn't what this is about.

Nico
-- 

On Thu, Mar 09, 2017 at 12:58:55PM -0500, Robert Haas wrote:
> On Thu, Mar 9, 2017 at 12:48 PM, Sven R. Kunze <srkunze@mail.de> wrote:
> > On 08.03.2017 20:48, Peter van Hardenberg wrote:
> >>
> >> Small point of order: YAML is not strictly a super-set of JSON.
> >
> > I haven't read the whole standard, but from what I can see the standard
> > considers JSON an official subset of itself:
> > http://www.yaml.org/spec/1.2/spec.html
> 
> But there's apparent sophistry, like this, in that spec:

I agree with you.  But beware, the IETF has had multiple threads with
thousands of posts in them about these sorts of issues.  If you're not
careful you'll have such a thread on this list too.  It would be very
sad not to only let a group that really cares have such threads instead.

:)

Nico
-- 

On Thu, Mar 09, 2017 at 07:12:07PM +0100, Sven R. Kunze wrote:
> From my day-to-day work I can tell, the date(time) type is the only missing
> piece of JSON to make it perfect for business applications (besides, maybe,
> a "currency" type).

And a binary type.  And a chunked-string type (to avoid having to escape
strings).  And an interval type.  And...

On 10.03.2017 20:28, Josh Berkus wrote:
> On 03/09/2017 10:12 AM, Sven R. Kunze wrote:
>>
>> SaltStack uses YAML for their tools, too. I personally can empathize
>> with them (as a user of configuration management) about this as writing
>> JSON would be nightmare with all the quoting, commas, curly braces etc.
>> But that's my own preference maybe.
> Yes, but automated tools can easily convert between JSON and
> newline-delimited YAML and back.

Sure. That wasn't point, though.


Sven

On 13.03.2017 07:24, Nico Williams wrote:
> On Thu, Mar 09, 2017 at 07:12:07PM +0100, Sven R. Kunze wrote:
>>  From my day-to-day work I can tell, the date(time) type is the only missing
>> piece of JSON to make it perfect for business applications (besides, maybe,
>> a "currency" type).
> And a binary type.  And a chunked-string type (to avoid having to escape
> strings).  And an interval type.  And...

YMMV but I tend to say that those aren't the usual types of a business 
application where I come from.

Answering questions like "how many" (integer), "what" (text) and "when" 
(date) is far more common than "give me that binary blob" at least in 
the domain where I work. Never had the necessity for an interval type; 
usually had a start and end value where the "interval" was derived from 
those values.

On 3/7/17 11:05 PM, David Steele wrote:
> On 3/7/17 11:38 AM, Andres Freund wrote:
> 
> <...>
> 
>>> We have a plenty of time and we dedicate one full-time developer for
>>> this project.
>>
>> How about having that, and perhaps others, developer participate in
>> reviewing patches and getting to the bottom of the commitfest?  Should
>> we end up being done early, we can look at this patch...  There's not
>> been review activity corresponding to the amount of submissions from
>> pgpro...
> 
> This patch has been moved to CF 2017-07.

I did not manage to move this patch when I said had.  It is now moved.

Thank,
-- 
-David
david@pgmasters.net

On 3/15/17 11:56, David Steele wrote:
>> This patch has been moved to CF 2017-07.
> 
> I did not manage to move this patch when I said had.  It is now moved.

Unsurprisingly, this patch needs a major rebase.

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

> On 15 Aug 2017, at 04:30, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>
> On 3/15/17 11:56, David Steele wrote:
>>> This patch has been moved to CF 2017-07.
>>
>> I did not manage to move this patch when I said had.  It is now moved.
>
> Unsurprisingly, this patch needs a major rebase.

Can we expect a rebased version of this patch for this commitfest?  Since it’s
a rather large feature it would be good to get it in as early as we can in the
process.

cheers ./daniel

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Sep 15, 2017 at 10:10 AM, Daniel Gustafsson <daniel@yesql.se> wrote:
> Can we expect a rebased version of this patch for this commitfest?  Since it’s
> a rather large feature it would be good to get it in as early as we can in the
> process.

Again, given that this needs a "major" rebase and hasn't been updated
in a month, and given that the CF is already half over, this should
just be bumped to the next CF.  We're supposed to be trying to review
things that were ready to go by the start of the CF, not the end.

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Sep 15, 2017 at 7:31 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Sep 15, 2017 at 10:10 AM, Daniel Gustafsson <daniel@yesql.se> wrote:
>> Can we expect a rebased version of this patch for this commitfest?  Since it’s
>> a rather large feature it would be good to get it in as early as we can in the
>> process.
>
> Again, given that this needs a "major" rebase and hasn't been updated
> in a month, and given that the CF is already half over, this should
> just be bumped to the next CF.  We're supposed to be trying to review
> things that were ready to go by the start of the CF, not the end.

We are supporting v10 branch in our github repository
https://github.com/postgrespro/sqljson/tree/sqljson_v10

Since the first post we made a lot of changes, mostly because of
better understanding the standard and availability of technical report
(http://standards.iso.org/ittf/PubliclyAvailableStandards/c067367_ISO_IEC_TR_19075-6_2017.zip).
Most important are:

1.We abandoned FORMAT support, which could confuse our users, since we
have data types json[b].

2. We use XMLTABLE infrastructure, extended for  JSON_TABLE support.

3. Reorganize commits, so we could split one big patch by several
smaller patches, which could be reviewed independently.

4. The biggest problem is documentation, we are working on it.

Nikita will submit patches soon.

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 15.09.2017 22:36, Oleg Bartunov wrote:

> On Fri, Sep 15, 2017 at 7:31 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Fri, Sep 15, 2017 at 10:10 AM, Daniel Gustafsson <daniel@yesql.se> wrote:
>>> Can we expect a rebased version of this patch for this commitfest?  Since it’s
>>> a rather large feature it would be good to get it in as early as we can in the
>>> process.
>> Again, given that this needs a "major" rebase and hasn't been updated
>> in a month, and given that the CF is already half over, this should
>> just be bumped to the next CF.  We're supposed to be trying to review
>> things that were ready to go by the start of the CF, not the end.
> We are supporting v10 branch in our github repository
> https://github.com/postgrespro/sqljson/tree/sqljson_v10
>
> Since the first post we made a lot of changes, mostly because of
> better understanding the standard and availability of technical report
> (http://standards.iso.org/ittf/PubliclyAvailableStandards/c067367_ISO_IEC_TR_19075-6_2017.zip).
> Most important are:
>
> 1.We abandoned FORMAT support, which could confuse our users, since we
> have data types json[b].
>
> 2. We use XMLTABLE infrastructure, extended for  JSON_TABLE support.
>
> 3. Reorganize commits, so we could split one big patch by several
> smaller patches, which could be reviewed independently.
>
> 4. The biggest problem is documentation, we are working on it.
>
> Nikita will submit patches soon.

Attached archive with 9 patches rebased onto latest master.

0001-jsonpath-v02.patch:
  - jsonpath type
  - jsonpath execution on jsonb type
  - jsonpath operators for jsonb type
  - GIN support for jsonpath operators

0002-jsonpath-json-v02.patch:
  - jsonb-like iterators for json type
  - jsonpath execution on json type
  - jsonpath operators for json type

0003-jsonpath-extensions-v02.patch:
0004-jsonpath-extensions-tests-for-json-v02.patch:
  - some useful standard extensions with tests
  
0005-sqljson-v02.patch:
  - SQL/JSON constructors (JSON_OBJECT[AGG], JSON_ARRAY[AGG])
  - SQL/JSON query functions (JSON_VALUE, JSON_QUERY, JSON_EXISTS)
  - IS JSON predicate

0006-sqljson-json-v02.patch:
  - SQL/JSON support for json type and tests

0007-json_table-v02.patch:
  - JSON_TABLE using XMLTABLE infrastructure

0008-json_table-json-v02.patch:
  - JSON_TABLE support for json type

0009-wip-extensions-v02.patch:
  - FORMAT JSONB
  - jsonb to/from bytea casts
  - jsonpath operators
  - some unfinished jsonpath extensions


Originally, JSON path was implemented only for jsonb type, and I decided to
add jsonb-like iterators for json type for json support implementation with
minimal changes in JSON path code.  This solution (see jsonpath_json.c from
patch 0002) looks a little dubious to me, so I separated json support into
independent patches.

The last WIP patch 0009 is unfinished and contains a lot of FIXMEs.  But
the ability to use arbitrary Postgres operators in JSON path with explicitly
specified  types is rather interesting, and I think it should be shown now
to get a some kind of pre-review.

We are supporting v11 and v10 branches in our github repository:

https://github.com/postgrespro/sqljson/tree/sqljson
https://github.com/postgrespro/sqljson/tree/sqljson_wip
https://github.com/postgrespro/sqljson/tree/sqljson_v10
https://github.com/postgrespro/sqljson/tree/sqljson_v10_wip

Attached patches can be produced simply by combining groups of consecutive
commits from these branches.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Nikita Glukhov wrote:

> 0007-json_table-v02.patch:
>  - JSON_TABLE using XMLTABLE infrastructure
> 
> 0008-json_table-json-v02.patch:
>  - JSON_TABLE support for json type

I'm confused ... why are these two patches and not a single one?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 18.09.2017 00:38, Alvaro Herrera wrote:

> Nikita Glukhov wrote:
>
>> 0007-json_table-v02.patch:
>>   - JSON_TABLE using XMLTABLE infrastructure
>>
>> 0008-json_table-json-v02.patch:
>>   - JSON_TABLE support for json type
> I'm confused ... why are these two patches and not a single one?
>
As I sad before, json support in jsonpath looks a bit dubious to me.  So if
patch no. 2 will not be accepted, then patches no. 4, 6, 8 should also be
simply skipped.  But, of course, patches 1 and 2, 3 and 4, 5 and 6, 7 and 8
can be combined.

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



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 29.09.2017 12:59, Pavel Stehule wrote:

Hi

2017-09-16 1:31 GMT+02:00 Nikita Glukhov <n.gluhov@postgrespro.ru>:

On 15.09.2017 22:36, Oleg Bartunov wrote:

On Fri, Sep 15, 2017 at 7:31 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Sep 15, 2017 at 10:10 AM, Daniel Gustafsson <daniel@yesql.se> wrote:

Can we expect a rebased version of this patch for this commitfest?  Since it’s
a rather large feature it would be good to get it in as early as we can in the
process.

Again, given that this needs a "major" rebase and hasn't been updated
in a month, and given that the CF is already half over, this should
just be bumped to the next CF.  We're supposed to be trying to review
things that were ready to go by the start of the CF, not the end.

We are supporting v10 branch in our github repository
https://github.com/postgrespro/sqljson/tree/sqljson_v10

Since the first post we made a lot of changes, mostly because of
better understanding the standard and availability of technical report
(http://standards.iso.org/ittf/PubliclyAvailableStandards/c067367_ISO_IEC_TR_19075-6_2017.zip).
Most important are:

1.We abandoned FORMAT support, which could confuse our users, since we
have data types json[b].

2. We use XMLTABLE infrastructure, extended for  JSON_TABLE support.

3. Reorganize commits, so we could split one big patch by several
smaller patches, which could be reviewed independently.

4. The biggest problem is documentation, we are working on it.

Nikita will submit patches soon.

Attached archive with 9 patches rebased onto latest master.

0001-jsonpath-v02.patch:
 - jsonpath type
 - jsonpath execution on jsonb type
 - jsonpath operators for jsonb type
 - GIN support for jsonpath operators

0002-jsonpath-json-v02.patch:
 - jsonb-like iterators for json type
 - jsonpath execution on json type
 - jsonpath operators for json type

0003-jsonpath-extensions-v02.patch:
0004-jsonpath-extensions-tests-for-json-v02.patch:
 - some useful standard extensions with tests
 0005-sqljson-v02.patch:
 - SQL/JSON constructors (JSON_OBJECT[AGG], JSON_ARRAY[AGG])
 - SQL/JSON query functions (JSON_VALUE, JSON_QUERY, JSON_EXISTS)
 - IS JSON predicate

0006-sqljson-json-v02.patch:
 - SQL/JSON support for json type and tests

0007-json_table-v02.patch:
 - JSON_TABLE using XMLTABLE infrastructure

0008-json_table-json-v02.patch:
 - JSON_TABLE support for json type

0009-wip-extensions-v02.patch:
 - FORMAT JSONB
 - jsonb to/from bytea casts
 - jsonpath operators
 - some unfinished jsonpath extensions


Originally, JSON path was implemented only for jsonb type, and I decided to
add jsonb-like iterators for json type for json support implementation with
minimal changes in JSON path code.  This solution (see jsonpath_json.c from
patch 0002) looks a little dubious to me, so I separated json support into
independent patches.

The last WIP patch 0009 is unfinished and contains a lot of FIXMEs.  But
the ability to use arbitrary Postgres operators in JSON path with explicitly
specified  types is rather interesting, and I think it should be shown now
to get a some kind of pre-review.

We are supporting v11 and v10 branches in our github repository:

https://github.com/postgrespro/sqljson/tree/sqljson
https://github.com/postgrespro/sqljson/tree/sqljson_wip
https://github.com/postgrespro/sqljson/tree/sqljson_v10
https://github.com/postgrespro/sqljson/tree/sqljson_v10_wip

Attached patches can be produced simply by combining groups of consecutive
commits from these branches.

I have some free time now. Is it last version?

Regards

Pavel

On 29.09.2017 20:07, Pavel Stehule wrote:

2017-09-29 12:15 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

2017-09-29 12:09 GMT+02:00 Nikita Glukhov <n.gluhov@postgrespro.ru>:

I have some free time now. Is it last version?

Regards

Pavel

Yes, this is still the latest version. Now I am working only on unfinished WIP
patch no. 9, but I think it should be reviewed the last.

ok

Thank you

I have few queries and notes

1. Why first patch holds Gin related functionality? Can be it separated?

2. Why Json path functions starts by "_" ? These functions are not removed by other patches.

3. What is base for jsonpath-extensions? ANSI/SQL?

This patch is pretty big - so I propose to push JSONPath and SQL/JSON related patches first, and then in next iteration to push JSON_TABLE patch. Is it acceptable strategy?

I am sure so JSON_TABLE is pretty important function, but it is pretty complex too (significantly more complex than XMLTABLE), so it can be practiacal to move this function to separate project. I hope so all patches will be merged in release 11 time.

Hi, hackers!

I have a question about transformation of JSON constructors into executor nodes.

In first letter in this thread we wrote:   JSON_OBJECT(), JSON_ARRAY() constructors and IS JSON predicate are
transformedinto raw function calls.
 

Here is an example explaining what it means:

=# CREATE VIEW json_object_view AS
SELECT JSON_OBJECT('foo': 1, 'bar': '[1,2]' FORMAT JSON RETURNING text);
CREATE VIEW
=# \sv json_object_view
CREATE OR REPLACE VIEW public.json_object_view AS SELECT json_build_object_ext(false, false, 'foo', 1, 'bar',
'[1,2]'::text::json)::text

As you can see JSON_OBJECT() was transformed into a call on new function
json_build_object_ext(), which shares a code with existing json_build_object()
but differs from it only by two additional boolean parameters for
representation of  {WITH|WITHOUT} UNIQUE [KEYS] and {NULL|ABSENT} ON NULL
clauses.  Information about FORMAT, RETURNING clauses was lost, since they
were transformed into casts.

Other constructors are transformed similary:
JSON_ARRAY()     => json[b]_build_array_ext(boolean, VARIADIC any)
JSON_OBJECTAGG() => json[b]_objectagg(any, any, boolean, boolean)
JSON_ARRAYAGG()  => json[b]_agg[_strict](any)

Also there is a variant of JSON_ARRAY() with subquery which transformed into a
subselect with json[b]_agg():
=# CREATE VIEW json_array_view AS SELECT JSON_ARRAY(SELECT generate_series(1,3));
CREATE VIEW
=# \sv json_array_view
CREATE OR REPLACE VIEW public.json_array_view AS SELECT ( SELECT json_agg_strict(q.a)           FROM ( SELECT
generate_series(1,3) AS generate_series) q(a))
 



And here is my question: is it acceptable to do such transformations?
And if is not acceptable (it seemed unacceptable to us from the beginning,
but we did not have time for correct implementation), how should JSON
constructor nodes look like?


The simplest solution that I can propose is to save both transformed
expressions in existing JsonObjectCtor/JsonArrayCtor nodes which exist
now only in untransformed trees.  Whole untransformed JsonXxxCtor node
will be used for displaying, transformed expression -- for execution only.

But it will not work for aggregates, because they are transformed into a
Aggref/WindowFunc node.  Information needed for correct displaying should be
saved somewhere in these standard nodes.

And for subquery variant of JSON_ARRAY I can only offer to leave transformation
into a subselect with JSON_ARRAYAGG():
JSON_ARRAY(query) => (SELECT JSON_ARRAYAGG(bar) FROM (query) foo(bar))

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



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Could someone clarify the status of this patch set?  It has been in
"Waiting" mode since the previous CF and no new patch, just a few
questions from the author.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017-02-28 20:08, Oleg Bartunov wrote:
> Attached patch is an implementation of SQL/JSON data model from SQL-2016
> standard (ISO/IEC 9075-2:2016(E))

I've faintly started looking into this.

> We created repository for reviewing (ask for write access) -
> https://github.com/postgrespro/sqljson/tree/sqljson

> Examples of usage can be found in src/test/regress/sql/sql_json.sql

> The whole documentation about json support should be reorganized and added,
> and we plan to do this before release. We need help of community here.


> The standard describes SQL/JSON path language, which used by SQL/JSON query
> operators to query JSON. It defines path language as string literal. We
> implemented the path language as  JSONPATH data type, since other
> approaches are not friendly to planner and executor.

I was a bit sad to discover that I can't
PREPARE jsq AS SELECT JSON_QUERY('{}', $1);
I assume because of this part of the updated grammar:
json_path_specification:
    Sconst         { $$ = $1; }
   ;

Would it make sense, fundamentally, to allow variables there? After 
Andrew Gierth's analysis of this grammar problem, I understand that it's 
not reasonable to expect JSON_TABLE() to support variable jsonpaths, but 
maybe it would be feasible for everything else? From Andrew's changes to 
the new grammar (see attached) it seems to me that at least that part is 
possible. Or should I forget about trying to implement the other part?

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 03.11.2017 00:32, Piotr Stefaniak wrote:

> On 2017-02-28 20:08, Oleg Bartunov wrote:
>> The standard describes SQL/JSON path language, which used by SQL/JSON query
>> operators to query JSON. It defines path language as string literal. We
>> implemented the path language as  JSONPATH data type, since other
>> approaches are not friendly to planner and executor.
> I was a bit sad to discover that I can't
> PREPARE jsq AS SELECT JSON_QUERY('{}', $1);
> I assume because of this part of the updated grammar:
> json_path_specification:
>      Sconst         { $$ = $1; }
>     ;
>
> Would it make sense, fundamentally, to allow variables there? After
> Andrew Gierth's analysis of this grammar problem, I understand that it's
> not reasonable to expect JSON_TABLE() to support variable jsonpaths, but
> maybe it would be feasible for everything else? From Andrew's changes to
> the new grammar (see attached) it seems to me that at least that part is
> possible. Or should I forget about trying to implement the other part?
By standard only string literals can be used in JSON path specifications.
But of course it is possible to allow to use variable jsonpath 
expressions in
SQL/JSON functions.

Attached patch implements this feature for JSON query functions, 
JSON_TABLE is
not supported now because it needs some refactoring.

I have pushed this commit to the separate branch because it is not 
finished yet:
https://github.com/postgrespro/sqljson/tree/sqljson_variable_json_path

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Nov 3, 2017 at 11:29 AM, Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
> By standard only string literals can be used in JSON path specifications.
> But of course it is possible to allow to use variable jsonpath expressions
> in
> SQL/JSON functions.
>
> Attached patch implements this feature for JSON query functions, JSON_TABLE
> is
> not supported now because it needs some refactoring.
>
> I have pushed this commit to the separate branch because it is not finished
> yet:
> https://github.com/postgrespro/sqljson/tree/sqljson_variable_json_path

The patch sent previously does not directly apply on HEAD, and as far
as I can see the last patch set published on
https://www.postgresql.org/message-id/2361ae4a-66b1-c6c5-ea6a-84851a1c08b5@postgrespro.ru
has rotten. Could you send a new patch set?

About the patch set, I had a look at the first patch which is not that
heavy, however it provides zero documentation, close to zero comments,
but adds more than 500 lines of code. I find that a bit hard to give
an opinion on, having commit messages associated to each patch would
be also nice. This way, reviewers can figure what's going out in this
mess and provide feedback. Making things incremental is welcome as
well, for example in the first patch I have a hard way finding out why
timestamps are touched to begin with.

The patch is already marked as "waiting on author" for more than one month.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Nov 3, 2017 at 12:07 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> The patch sent previously does not directly apply on HEAD, and as far
> as I can see the last patch set published on
> https://www.postgresql.org/message-id/2361ae4a-66b1-c6c5-ea6a-84851a1c08b5@postgrespro.ru
> has rotten. Could you send a new patch set?
>
> About the patch set, I had a look at the first patch which is not that
> heavy, however it provides zero documentation, close to zero comments,
> but adds more than 500 lines of code. I find that a bit hard to give
> an opinion on, having commit messages associated to each patch would
> be also nice. This way, reviewers can figure what's going out in this
> mess and provide feedback. Making things incremental is welcome as
> well, for example in the first patch I have a hard way finding out why
> timestamps are touched to begin with.

My mistake here, only the first patch adds 8,200 lines of code. This
makes the lack of comments and docs even worse.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 03.11.2017 15:07, Michael Paquier wrote:

> On Fri, Nov 3, 2017 at 11:29 AM, Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
>> By standard only string literals can be used in JSON path specifications.
>> But of course it is possible to allow to use variable jsonpath expressions
>> in
>> SQL/JSON functions.
>>
>> Attached patch implements this feature for JSON query functions, JSON_TABLE
>> is
>> not supported now because it needs some refactoring.
>>
>> I have pushed this commit to the separate branch because it is not finished
>> yet:
>> https://github.com/postgrespro/sqljson/tree/sqljson_variable_json_path
> The patch sent previously does not directly apply on HEAD, and as far
> as I can see the last patch set published on
> https://www.postgresql.org/message-id/2361ae4a-66b1-c6c5-ea6a-84851a1c08b5@postgrespro.ru
> has rotten. Could you send a new patch set?
Attached patch set rebased onto current master.
Branches in our github repository also updated.

> About the patch set, I had a look at the first patch which is not that
> heavy, however it provides zero documentation, close to zero comments,
> but adds more than 500 lines of code. I find that a bit hard to give
> an opinion on, having commit messages associated to each patch would
> be also nice. This way, reviewers can figure what's going out in this
> mess and provide feedback.
Sorry that comments and commit messages are still absent. I am going to 
do it
in the next version of these patches where SQL/JSON constructors displaying
will be fixed.

>   Making things incremental is welcome as
> well, for example in the first patch I have a hard way finding out why
> timestamps are touched to begin with.
Timestamp's code was touched to add support of two features needed for 
SQL/JSON
  .datetime() item method by standard:
  - TZH and TZM template patterns
  - datetime components recognition

I absolutely agree that this should be in a separate patch.

> The patch is already marked as "waiting on author" for more than one month.
-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 04.11.2017 01:52, Nikita Glukhov wrote:

>> On 03.11.2017 15:07, Michael Paquier wrote:
>>
>> About the patch set, I had a look at the first patch which is not that
>> heavy, however it provides zero documentation, close to zero comments,
>> but adds more than 500 lines of code. I find that a bit hard to give
>> an opinion on, having commit messages associated to each patch would
>> be also nice. This way, reviewers can figure what's going out in this
>> mess and provide feedback.
> Sorry that comments and commit messages are still absent. I am going 
> to do it
> in the next version of these patches where SQL/JSON constructors 
> displaying
> will be fixed.
>
Attached the new version of the patches where displaying of SQL/JSON
constructor nodes was fixed.  I decided not to invent new nodes but to 
extend
existing FuncExpr, Aggref, WindowFunc nodes with new formatting fields that
give us ability to override default displaying in ruleutils.c.  Also new
invisible CoercionForm was added for hiding casts in which FORMAT and 
RETUNING
clauses are transformed.

>>   Making things incremental is welcome as
>> well, for example in the first patch I have a hard way finding out why
>> timestamps are touched to begin with.
> Timestamp's code was touched to add support of two features needed for 
> SQL/JSON
>  .datetime() item method by standard:
>  - TZH and TZM template patterns
>  - datetime components recognition
>
> I absolutely agree that this should be in a separate patch.
>
TZH and TZM template patterns were moved into a separate patch.

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

On Wed, Nov 15, 2017 at 10:17 AM, Nikita Glukhov
<n.gluhov@postgrespro.ru> wrote:
> Attached the new version of the patches where displaying of SQL/JSON
> constructor nodes was fixed.  I decided not to invent new nodes but to
> extend
> existing FuncExpr, Aggref, WindowFunc nodes with new formatting fields that
> give us ability to override default displaying in ruleutils.c.  Also new
> invisible CoercionForm was added for hiding casts in which FORMAT and
> RETUNING
> clauses are transformed.

Okay, I can see that the patch is still in the same situation two
weeks after I looked at it first, so I am marking it as returned with
feedback. This needs to be broken down, and get documentation. At this
point getting a review out of this patch is not something I'd
recommend until it is put in a shape that makes it easier. Please also
help in reviewing other's patches, yours here is very large.
-- 
Michael

On 29.11.2017 05:24, Michael Paquier wrote:

> On Wed, Nov 15, 2017 at 10:17 AM, Nikita Glukhov
> <n.gluhov@postgrespro.ru> wrote:
>> Attached the new version of the patches where displaying of SQL/JSON
>> constructor nodes was fixed.  I decided not to invent new nodes but to
>> extend
>> existing FuncExpr, Aggref, WindowFunc nodes with new formatting fields that
>> give us ability to override default displaying in ruleutils.c.  Also new
>> invisible CoercionForm was added for hiding casts in which FORMAT and
>> RETUNING
>> clauses are transformed.
> Okay, I can see that the patch is still in the same situation two
> weeks after I looked at it first, so I am marking it as returned with
> feedback. This needs to be broken down, and get documentation. At this
> point getting a review out of this patch is not something I'd
> recommend until it is put in a shape that makes it easier. Please also
> help in reviewing other's patches, yours here is very large.

Attached new version of patches:

1. Added some comments for the jsonpath code and some documentation for
jsonpath operators and opclasses.  Sorry that complete documentation for
jsonpath itself is still missing.

2. Added support for custom user-defined item methods and functions in 
jsonpath.
This feature allows us to move our non-standard methods (map, reduce, 
fold, min,
max) to the extension contrib/jsnopathx. Now user can implement all standard
JavaScript array methods by himself.

3. Added variable jsonpath specifications in SQL/JSON functions.

4. Added subtransactions inside PG_TRY/PG_CATCH block that is used for 
ON ERROR
clause support in JSON_EXISTS, JSON_VALUE, JSON_QUERY (see ExecEvalJson() in
src/backend/executor/execExpeInterp.c).


The last addition is the most questionable.  By using standard ON ERROR 
сlause
we can specify default value for expression when an error occurs during 
casting
JSON data to the target SQL type.  Cast expressions can contain 
arbitrary user
functions, so they should be executed inside own subtransaction like it 
is done
in plpgsql (see exec_stmt_block()).  Subtransaction start obviously 
introduces
significant performance overhead (except the case when ERROR ON ERROR is
used and error handling is unnecessary):

=# EXPLAIN (ANALYZE)
    SELECT JSON_VALUE(jsonb '1', '$' RETURNING int ERROR ON ERROR)
    FROM generate_series(1, 1000000);
  ...
  Execution time: 395.238 ms

=# EXPLAIN (ANALYZE)
    SELECT JSON_VALUE(jsonb '1', '$' RETURNING int)
    FROM generate_series(1, 1000000);
  ...
  Execution time: 914.850 ms

To partially eliminate this overhead, I'm trying to examine cast-expression
volatility:
  * mutable -- subtransaction is started
  * stable -- subtransaction is not started, only new ResourceOwner is 
created
  * immutable -- ResourceOwner is not even created
But don't know if it is correct to rely on volatility here.  And also I 
doubt
that we can start multiple subtransactions (for each SQL/JSON function
  evaluation) within a single SELECT statement.

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

On Tue, Jan 2, 2018 at 10:47 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
> 2018-01-02 3:04 GMT+01:00 Nikita Glukhov <n.gluhov@postgrespro.ru>:
>>
>> On 29.11.2017 05:24, Michael Paquier wrote:
>>
>>> On Wed, Nov 15, 2017 at 10:17 AM, Nikita Glukhov
>>> <n.gluhov@postgrespro.ru> wrote:
>>>>
>>>> Attached the new version of the patches where displaying of SQL/JSON
>>>> constructor nodes was fixed.  I decided not to invent new nodes but to
>>>> extend
>>>> existing FuncExpr, Aggref, WindowFunc nodes with new formatting fields
>>>> that
>>>> give us ability to override default displaying in ruleutils.c.  Also new
>>>> invisible CoercionForm was added for hiding casts in which FORMAT and
>>>> RETUNING
>>>> clauses are transformed.
>>>
>>> Okay, I can see that the patch is still in the same situation two
>>> weeks after I looked at it first, so I am marking it as returned with
>>> feedback. This needs to be broken down, and get documentation. At this
>>> point getting a review out of this patch is not something I'd
>>> recommend until it is put in a shape that makes it easier. Please also
>>> help in reviewing other's patches, yours here is very large.
>>
>>
>> Attached new version of patches:
>>
>> 1. Added some comments for the jsonpath code and some documentation for
>> jsonpath operators and opclasses.  Sorry that complete documentation for
>> jsonpath itself is still missing.
>>
>> 2. Added support for custom user-defined item methods and functions in
>> jsonpath.
>> This feature allows us to move our non-standard methods (map, reduce,
>> fold, min,
>> max) to the extension contrib/jsnopathx. Now user can implement all
>> standard
>> JavaScript array methods by himself.
>>
>> 3. Added variable jsonpath specifications in SQL/JSON functions.
>>
>> 4. Added subtransactions inside PG_TRY/PG_CATCH block that is used for ON
>> ERROR
>> clause support in JSON_EXISTS, JSON_VALUE, JSON_QUERY (see ExecEvalJson()
>> in
>> src/backend/executor/execExpeInterp.c).
>>
>>
>> The last addition is the most questionable.  By using standard ON ERROR
>> сlause
>> we can specify default value for expression when an error occurs during
>> casting
>> JSON data to the target SQL type.  Cast expressions can contain arbitrary
>> user
>> functions, so they should be executed inside own subtransaction like it is
>> done
>> in plpgsql (see exec_stmt_block()).  Subtransaction start obviously
>> introduces
>> significant performance overhead (except the case when ERROR ON ERROR is
>> used and error handling is unnecessary):
>>
>> =# EXPLAIN (ANALYZE)
>>    SELECT JSON_VALUE(jsonb '1', '$' RETURNING int ERROR ON ERROR)
>>    FROM generate_series(1, 1000000);
>>  ...
>>  Execution time: 395.238 ms
>>
>> =# EXPLAIN (ANALYZE)
>>    SELECT JSON_VALUE(jsonb '1', '$' RETURNING int)
>>    FROM generate_series(1, 1000000);
>>  ...
>>  Execution time: 914.850 ms
>>
>> To partially eliminate this overhead, I'm trying to examine
>> cast-expression
>> volatility:
>>  * mutable -- subtransaction is started
>>  * stable -- subtransaction is not started, only new ResourceOwner is
>> created
>>  * immutable -- ResourceOwner is not even created
>> But don't know if it is correct to rely on volatility here.  And also I
>> doubt
>> that we can start multiple subtransactions (for each SQL/JSON function
>>  evaluation) within a single SELECT statement.
>>
>
> I am looking on this patch set and it looks very well.
>
> Personally I dislike any extensions against SQL/JSON in this patch. And
> there is lot of extensions there. It doesn't mean so these extensions are
> bad, but it should be passed as next step and there should be separate
> discussion related to these extensions.
>
> Please, can you reduce this patch to only SQL/JSON part?

+1, our goal is to push the standard to PG 11, which is more or less realistic.
Nikita will rearrange the patch set, so patches 1, 2, 4, 7, 8, 9, 10,
11, 12, which
implement SQL/JSON could be applied without extra patches.

Patches 5,6 are desirable, since we can implement custom operators. This is
very important for postgres, which is known as extensible database with rich set
of extensions. Think about geojson with spatial operators or  array
operators, for
example. But I agree, it's subject of separate thread.

In very extreme case, we could commit for PG 11 only jsonpath-related patches
1,2 and probably 4.  I think, that jsonpath is what we really miss in postgres.

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

On 01/02/2018 02:44 PM, Oleg Bartunov wrote:
> On Tue, Jan 2, 2018 at 10:47 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

>> I am looking on this patch set and it looks very well.
>>
>> Personally I dislike any extensions against SQL/JSON in this patch. And
>> there is lot of extensions there. It doesn't mean so these extensions are
>> bad, but it should be passed as next step and there should be separate
>> discussion related to these extensions.
>>
>> Please, can you reduce this patch to only SQL/JSON part?
> +1, our goal is to push the standard to PG 11, which is more or less realistic.
> Nikita will rearrange the patch set, so patches 1, 2, 4, 7, 8, 9, 10,
> 11, 12, which
> implement SQL/JSON could be applied without extra patches.
>
> Patches 5,6 are desirable, since we can implement custom operators. This is
> very important for postgres, which is known as extensible database with rich set
> of extensions. Think about geojson with spatial operators or  array
> operators, for
> example. But I agree, it's subject of separate thread.
>
> In very extreme case, we could commit for PG 11 only jsonpath-related patches
> 1,2 and probably 4.  I think, that jsonpath is what we really miss in postgres.


That seems a bit pessimistic. I hope we can do lots better.

It looks to me like patches 1, 7 and 8 can stand alone, and should be
submitted separately, and we should try to get them committed early.
These are all small patches - a couple of hundred lines each.

Patches 2, 3, and 4 should come next - I included patch 3 because I
think GIN indexing is going to be critical to success.

After that 9, 10, 11 and 12.

I don't have a problem with the rest, but they should probably have a
lower priority. If we can get to them well and good.

We should stop use the word 'extension' when we don't mean what Postgres
calls an extension (which is only patch 14 in this case). Call it an
addition or extra feature or something else. Otherwise it gets confusing.

I'm not 100% clear on why we're adding jsonpathx as an extension,
though. Do we not think most json users will want to use map, reduce etc.?



cheers

andrew

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

On 01/02/2018 03:48 PM, Pavel Stehule wrote:
>
>
> 2018-01-02 21:39 GMT+01:00 Andrew Dunstan
> <andrew.dunstan@2ndquadrant.com <mailto:andrew.dunstan@2ndquadrant.com>>:
>
>
>
>     On 01/02/2018 02:44 PM, Oleg Bartunov wrote:
>     > On Tue, Jan 2, 2018 at 10:47 AM, Pavel Stehule
>     <pavel.stehule@gmail.com <mailto:pavel.stehule@gmail.com>> wrote:
>
>     >> I am looking on this patch set and it looks very well.
>     >>
>     >> Personally I dislike any extensions against SQL/JSON in this
>     patch. And
>     >> there is lot of extensions there. It doesn't mean so these
>     extensions are
>     >> bad, but it should be passed as next step and there should be
>     separate
>     >> discussion related to these extensions.
>     >>
>     >> Please, can you reduce this patch to only SQL/JSON part?
>     > +1, our goal is to push the standard to PG 11, which is more or
>     less realistic.
>     > Nikita will rearrange the patch set, so patches 1, 2, 4, 7, 8,
>     9, 10,
>     > 11, 12, which
>     > implement SQL/JSON could be applied without extra patches.
>     >
>     > Patches 5,6 are desirable, since we can implement custom
>     operators. This is
>     > very important for postgres, which is known as extensible
>     database with rich set
>     > of extensions. Think about geojson with spatial operators or  array
>     > operators, for
>     > example. But I agree, it's subject of separate thread.
>     >
>     > In very extreme case, we could commit for PG 11 only
>     jsonpath-related patches
>     > 1,2 and probably 4.  I think, that jsonpath is what we really
>     miss in postgres.
>
>
>     That seems a bit pessimistic. I hope we can do lots better.
>
>     It looks to me like patches 1, 7 and 8 can stand alone, and should be
>     submitted separately, and we should try to get them committed early.
>     These are all small patches - a couple of hundred lines each.
>
>     Patches 2, 3, and 4 should come next - I included patch 3 because I
>     think GIN indexing is going to be critical to success.
>
>     After that 9, 10, 11 and 12.
>
>     I don't have a problem with the rest, but they should probably have a
>     lower priority. If we can get to them well and good.
>
>     We should stop use the word 'extension' when we don't mean what
>     Postgres
>     calls an extension (which is only patch 14 in this case). Call it an
>     addition or extra feature or something else. Otherwise it gets
>     confusing.
>
>     I'm not 100% clear on why we're adding jsonpathx as an extension,
>     though. Do we not think most json users will want to use map,
>     reduce etc.?
>
>
> In this moment, there is lot of code, and we should be concentrated to
> merging the core of this feature. I am sure, so discussion about extra
> features will come, and will be more realistic and less nervous if
> SQL/JSON will be merged already.
>
> I looked to patch - and It is big, really big - we should to start
> with some important subset that we can understand and test well.


Sure, I agree, we should start with jsonpath, and then move on to 
SQL/JSON and then the json_table patches. Patches 5, 6, 13 and 14 would
come last. That's the order I suggested above. My question was whether
or not, when we finally get to jsonpathx we really want to make it an
extension. I can't see a very good reason for doing so.

cheers

andrew


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

On Tue, Jan 2, 2018 at 8:39 PM, Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:
>
>
> On 01/02/2018 02:44 PM, Oleg Bartunov wrote:
>> On Tue, Jan 2, 2018 at 10:47 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>>> I am looking on this patch set and it looks very well.
>>>
>>> Personally I dislike any extensions against SQL/JSON in this patch. And
>>> there is lot of extensions there. It doesn't mean so these extensions are
>>> bad, but it should be passed as next step and there should be separate
>>> discussion related to these extensions.
>>>
>>> Please, can you reduce this patch to only SQL/JSON part?
>> +1, our goal is to push the standard to PG 11, which is more or less realistic.
>> Nikita will rearrange the patch set, so patches 1, 2, 4, 7, 8, 9, 10,
>> 11, 12, which
>> implement SQL/JSON could be applied without extra patches.
>>
>> Patches 5,6 are desirable, since we can implement custom operators. This is
>> very important for postgres, which is known as extensible database with rich set
>> of extensions. Think about geojson with spatial operators or  array
>> operators, for
>> example. But I agree, it's subject of separate thread.
>>
>> In very extreme case, we could commit for PG 11 only jsonpath-related patches
>> 1,2 and probably 4.  I think, that jsonpath is what we really miss in postgres.
>
>
> That seems a bit pessimistic. I hope we can do lots better.

Would love too !

>
> It looks to me like patches 1, 7 and 8 can stand alone, and should be
> submitted separately, and we should try to get them committed early.
> These are all small patches - a couple of hundred lines each.

+1

>
> Patches 2, 3, and 4 should come next - I included patch 3 because I
> think GIN indexing is going to be critical to success.

agree, we can consider patch 4 later

>
> After that 9, 10, 11 and 12.


again,  10 , 12 may be considered later

>
> I don't have a problem with the rest, but they should probably have a
> lower priority. If we can get to them well and good.
>
> We should stop use the word 'extension' when we don't mean what Postgres
> calls an extension (which is only patch 14 in this case). Call it an
> addition or extra feature or something else. Otherwise it gets confusing.

+1, lets call 'extra'

>
> I'm not 100% clear on why we're adding jsonpathx as an extension,
> though. Do we not think most json users will want to use map, reduce etc.?

We decided to do that, since the whole patch set is already big.

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

On 03.01.2018 00:38, Oleg Bartunov wrote:
> On Tue, Jan 2, 2018 at 8:39 PM, Andrew Dunstan
> <andrew.dunstan@2ndquadrant.com> wrote:
>>
>> On 01/02/2018 02:44 PM, Oleg Bartunov wrote:
>>> On Tue, Jan 2, 2018 at 10:47 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>>>> I am looking on this patch set and it looks very well.
>>>>
>>>> Personally I dislike any extensions against SQL/JSON in this patch. And
>>>> there is lot of extensions there. It doesn't mean so these extensions are
>>>> bad, but it should be passed as next step and there should be separate
>>>> discussion related to these extensions.
>>>>
>>>> Please, can you reduce this patch to only SQL/JSON part?
>>> +1, our goal is to push the standard to PG 11, which is more or less realistic.
>>> Nikita will rearrange the patch set, so patches 1, 2, 4, 7, 8, 9, 10,
>>> 11, 12, which
>>> implement SQL/JSON could be applied without extra patches.
>>>
>>> Patches 5,6 are desirable, since we can implement custom operators. This is
>>> very important for postgres, which is known as extensible database with rich set
>>> of extensions. Think about geojson with spatial operators or  array
>>> operators, for
>>> example. But I agree, it's subject of separate thread.
>>>
>>> In very extreme case, we could commit for PG 11 only jsonpath-related patches
>>> 1,2 and probably 4.  I think, that jsonpath is what we really miss in postgres.
>>
>> That seems a bit pessimistic. I hope we can do lots better.
> Would love too !
>
>> It looks to me like patches 1, 7 and 8 can stand alone, and should be
>> submitted separately, and we should try to get them committed early.
>> These are all small patches - a couple of hundred lines each.
> +1
>
>> Patches 2, 3, and 4 should come next - I included patch 3 because I
>> think GIN indexing is going to be critical to success.
> agree, we can consider patch 4 later
>
>> After that 9, 10, 11 and 12.
>
> again,  10 , 12 may be considered later
>
>> I don't have a problem with the rest, but they should probably have a
>> lower priority. If we can get to them well and good.
>>
>> We should stop use the word 'extension' when we don't mean what Postgres
>> calls an extension (which is only patch 14 in this case). Call it an
>> addition or extra feature or something else. Otherwise it gets confusing.
> +1, lets call 'extra'
>
>> I'm not 100% clear on why we're adding jsonpathx as an extension,
>> though. Do we not think most json users will want to use map, reduce etc.?
> We decided to do that, since the whole patch set is already big.
>
>>
>>
>> cheers
>>
>> andrew
>>
>> --
>> Andrew Dunstan                https://www.2ndQuadrant.com
>> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>>

I have removed all extra features from the patch set, they can be found 
in our
github repository: https://github.com/postgrespro/sqljson/tree/sqljson_ext.

Now there are 10 patches which have the following dependencies:

1:
2: 1
3: 2
4: 2
5:
6:
7: 2, 5, 6
8: 7, 4
9: 7
10: 8, 9

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

On 01/02/2018 05:04 PM, Nikita Glukhov wrote:
>
> I have removed all extra features from the patch set, they can be
> found in our
> github repository:
> https://github.com/postgrespro/sqljson/tree/sqljson_ext.
>
> Now there are 10 patches which have the following dependencies:
>
> 1:
> 2: 1
> 3: 2
> 4: 2
> 5:
> 6:
> 7: 2, 5, 6
> 8: 7, 4
> 9: 7
> 10: 8, 9
>


OK. We need to get this into the commitfest. Preferably not all in one
piece. I would do:

1, 5, and 6  independently
2, 3 and 4 together
7 and 8 together
9 and 10 together

Those seem like digestible pieces.

Also, there is a spurious BOM at the start of
src/test/regress/sql/sqljson.sql in patch 7. Please fix that.

cheers

andrew

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

On 01/03/2018 11:04 AM, Oleg Bartunov wrote:
>
>
> On 3 Jan 2018 00:23, "Andrew Dunstan" <andrew.dunstan@2ndquadrant.com
> <mailto:andrew.dunstan@2ndquadrant.com>> wrote:
>
>
>
>     On 01/02/2018 05:04 PM, Nikita Glukhov wrote:
>     >
>     > I have removed all extra features from the patch set, they can be
>     > found in our
>     > github repository:
>     > https://github.com/postgrespro/sqljson/tree/sqljson_ext
>     <https://github.com/postgrespro/sqljson/tree/sqljson_ext>.
>     >
>     > Now there are 10 patches which have the following dependencies:
>     >
>     > 1:
>     > 2: 1
>     > 3: 2
>     > 4: 2
>     > 5:
>     > 6:
>     > 7: 2, 5, 6
>     > 8: 7, 4
>     > 9: 7
>     > 10: 8, 9
>     >
>
>
>     OK. We need to get this into the commitfest. Preferably not all in one
>     piece. I would do:
>
>     1, 5, and 6  independently
>     2, 3 and 4 together
>     7 and 8 together
>     9 and 10 together
>
>
> What's about 11,12 ? They are about json_table.



No, those are 9 and 10 in Nikita's new patchset.


cheers

andrew

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

On Sat, Jan 6, 2018 at 8:22 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Hi
>
> I am checking the JSONPath related code
>
> Questions, notes:
>
> 1. jsonpath operators are not consistent with any other .. json, xml .. I am
> missing ?, @> operátors

I have slides about jsonpath
http://www.sai.msu.su/~megera/postgres/talks/sqljson-pgconf.eu-2017.pdf

> 2. documentation issue - there is "'{"a":[1,2,3,4,5]}'::json *? '$.a[*] ? (@
>> 2)'" - operator *? doesn't exists

There are should be @? operator

> 3. operator @~ looks like too aggressive shortcut - should be better
> commented
>
> What is not clean, if jsonpath should to create some new operators for json,
> jsonb types? It is special filter, defined by type, so from my perspective
> the special operators are not necessary.

It's impossible to distinguish jsonpath from text, so introducing new operators
are easier than everytime explicitly specify jsonpath datatype.

>
> Regards
>
> Pavel
>
>

On 07.01.2018 00:22, Pavel Stehule wrote:

Hi

I try jsonpath on json

{
    "book":
    [
        {
            "title": "Beginning JSON",
            "author": "Ben Smith",
            "price": 49.99
        },
 
        {
            "title": "JSON at Work",
            "author": "Tom Marrs",
            "price": 29.99
        },
 
        {
            "title": "Learn JSON in a DAY",
            "author": "Acodemy",
            "price": 8.99
        },
 
        {
            "title": "JSON: Questions and Answers",
            "author": "George Duckett",
            "price": 6.00
        }
    ],
 
    "price range":
    {
        "cheap": 10.00,
        "medium": 20.00
    }
}

I am not jsonpath expert, so I can be bad

How I can get title of book with cost 6?

postgres=# select j @* '$.book[*] ? (@.price==6)' from test;
┌─────────────────────────────────────────────────────┐
│                      ?column?                       │
╞═════════════════════════════════════════════════════╡
│ {                                                  ↵│
│             "title": "JSON: Questions and Answers",↵│
│             "author": "George Duckett",            ↵│
│             "price": 6.00                          ↵│
│         }                                          ↵│
│                                                     │
└─────────────────────────────────────────────────────┘
(1 row)

-- not sure, if it is correct

postgres=# select j @* '$.book[*].title ? (@.price==6)' from test;
┌──────────┐
│ ?column? │
╞══════════╡
└──────────┘
(0 rows)

I found some examples, where the filter has bigger sense, but it is not supported

LINE 1: select j @* '$.book[?(@.price==6.00)].title' from test;
                    ^
DETAIL:  syntax error, unexpected '?' at or near "?"

On 07.01.2018 00:33, Pavel Stehule wrote:

2018-01-06 22:23 GMT+01:00 Nikita Glukhov <n.gluhov@postgrespro.ru>:

On 07.01.2018 00:22, Pavel Stehule wrote:

Hi

I try jsonpath on json

{
    "book":
    [
        {
            "title": "Beginning JSON",
            "author": "Ben Smith",
            "price": 49.99
        },
 
        {
            "title": "JSON at Work",
            "author": "Tom Marrs",
            "price": 29.99
        },
 
        {
            "title": "Learn JSON in a DAY",
            "author": "Acodemy",
            "price": 8.99
        },
 
        {
            "title": "JSON: Questions and Answers",
            "author": "George Duckett",
            "price": 6.00
        }
    ],
 
    "price range":
    {
        "cheap": 10.00,
        "medium": 20.00
    }
}

I am not jsonpath expert, so I can be bad

How I can get title of book with cost 6?

postgres=# select j @* '$.book[*] ? (@.price==6)' from test;
┌─────────────────────────────────────────────────────┐
│                      ?column?                       │
╞═════════════════════════════════════════════════════╡
│ {                                                  ↵│
│             "title": "JSON: Questions and Answers",↵│
│             "author": "George Duckett",            ↵│
│             "price": 6.00                          ↵│
│         }                                          ↵│
│                                                     │
└─────────────────────────────────────────────────────┘
(1 row)

-- not sure, if it is correct

postgres=# select j @* '$.book[*].title ? (@.price==6)' from test;
┌──────────┐
│ ?column? │
╞══════════╡
└──────────┘
(0 rows)

I found some examples, where the filter has bigger sense, but it is not supported

LINE 1: select j @* '$.book[?(@.price==6.00)].title' from test;
                    ^
DETAIL:  syntax error, unexpected '?' at or near "?"

".title" simply should go after the filter:

select j @* '$.book[*] ? (@.price==6.00).title' from test;

It is working, thank you.

and the form "$.book[?(@.price==6.00)].title" ? I found this example in some other SQL/JSON implementations.

On 07.01.2018 00:11, Pavel Stehule wrote:

2018-01-06 22:02 GMT+01:00 Oleg Bartunov <obartunov@gmail.com>:

On Sat, Jan 6, 2018 at 8:22 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Hi
>
> I am checking the JSONPath related code
>
> Questions, notes:
>
> 1. jsonpath operators are not consistent with any other .. json, xml .. I am
> missing ?, @> operátors

I have slides about jsonpath
http://www.sai.msu.su/~megera/postgres/talks/sqljson-pgconf.eu-2017.pdf

> 2. documentation issue - there is "'{"a":[1,2,3,4,5]}'::json *? '$.a[*] ? (@
>> 2)'" - operator *? doesn't exists

There are should be @? operator

> 3. operator @~ looks like too aggressive shortcut - should be better
> commented
>
> What is not clean, if jsonpath should to create some new operators for json,
> jsonb types? It is special filter, defined by type, so from my perspective
> the special operators are not necessary.

It's impossible to distinguish jsonpath from text, so introducing new operators
are easier than everytime explicitly specify jsonpath datatype.

There are two possible solutions - special operator or explicit casting. In this case I am not sure if special operator for this case is good solution. Probably nobody will use it - because there SQL/JSON functions, but I don't think so this inconsistency is correct.

I have not strong opinion about it - it will be hidden feature for almost all users.

On 01/02/2018 05:23 PM, Andrew Dunstan wrote:
>
> On 01/02/2018 05:04 PM, Nikita Glukhov wrote:
>> I have removed all extra features from the patch set, they can be
>> found in our
>> github repository:
>> https://github.com/postgrespro/sqljson/tree/sqljson_ext.
>>
>> Now there are 10 patches which have the following dependencies:
>>
>> 1:
>> 2: 1
>> 3: 2
>> 4: 2
>> 5:
>> 6:
>> 7: 2, 5, 6
>> 8: 7, 4
>> 9: 7
>> 10: 8, 9
>>
>
> OK. We need to get this into the commitfest. Preferably not all in one
> piece. I would do:
>
> 1, 5, and 6  independently
> 2, 3 and 4 together
> 7 and 8 together
> 9 and 10 together
>
> Those seem like digestible pieces.
>
> Also, there is a spurious BOM at the start of
> src/test/regress/sql/sqljson.sql in patch 7. Please fix that.
>


OK, an extended version of patch 1 has been committed. I'm working on
patches 2, 3, and 4 (the jsonpath patches) as time permits (and right
now time is very tight). Here are some very preliminary comments:

  * The documentation needs improvement. A para with contents of "TODO"
    is not acceptable.
  * I'm not generally a fan of using flex/bison for small languages like
    this. Something similar to what we're using for json itself (a
    simple lexer and a recursive descent parser) would be more suitable.
    Others might have different views.
  * The timestamp handling refactoring is a good thing but would
    probably be better done as a separate patch.
  * the code is pretty sparsely commented. Quite apart from other
    considerations that makes it harder to review.

I also note that the later patches have no documentation whatsoever.
That needs serious work, and if you want to get these patches in then
please supply some documentation ASAP. If you need help with English we
can work on that, but just throwing patches of this size and complexity
over the wall into the commitfest without any documentation is not the
way to proceed.

cheers

andrew

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

On 01/10/2018 01:37 AM, Oleg Bartunov wrote:
>
>
>
>         this. Something similar to what we're using for json itself (a
>         simple lexer and a recursive descent parser) would be more
>     suitable.
>
>
> flex/bison is right tool for jsonpath, which is complex thing. 



It's not really very complex. The bison grammar has 21 non-terminals. As
languages go that's not huge.

May main concerns are code size and performance. RD parsers are
typically very fast and compact unless they are badly written. There are
reasons that years ago gcc switched from using bison to using a hand cut
RD parser. I guess we wouldn't expect for the most part that jsonpath
expressions would need to be compiled per row, so maybe performance
isn't that critical. But if we do expect really dynamic jsonpath
expressions then we need to make sure we are as fast as we can get.

I guess if all you have is a hammer everything looks like a nail, but we
should not assume that bison is the answer to every parsing problem we have.

I'm not going to hold up the patch over this issue. I should probably
have looked closer and raised it months ago. But if and when I get time
I will look into some benchmarking.

>     I also note that the later patches have no documentation whatsoever.
>     That needs serious work, and if you want to get these patches in then
>     please supply some documentation ASAP. If you need help with
>     English we
>     can work on that, but just throwing patches of this size and
>     complexity
>     over the wall into the commitfest without any documentation is not the
>     way to proceed.
>
>
> Andrew, we are back from holidays and I will start writing on
> documentation. I have difficulty with design of documentation, since
> it's unclear to me how detailed it should be. I'm inclining to follow
> xml style of documentation, which is quite formal and could be more
> easy to write.


OK, good. The sooner the better though. Err on the side of more detail
please.

cheers

andrew


-- 

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

Hi,

This patchset currently has multiple CF entries:
https://commitfest.postgresql.org/17/1063/

and then subordinate ones like
https://commitfest.postgresql.org/17/1471/
https://commitfest.postgresql.org/17/1472/
https://commitfest.postgresql.org/17/1473/

Thus I'm marking this entry as returned with feedback.

- Andres

On 2018-03-02 10:31:34 +0300, Oleg Bartunov wrote:
> Right, we divided it to manageable pieces as Andrew suggested.

Please close the corresponding CF entry next time, if you do so. It's a
bit painful having to reconstruct such things out of numerous large
threads.

Greetings,

Andres Freund